Paraswap, AaveV2, Balancer, Yearn, DSR Filters #208
Conversation
olivdb
force-pushed
the
minimal-wallet/defi-filters
branch
from
9f0b470
to
854caff
Compare
olivdb
changed the title
olivdb
force-pushed
the
minimal-wallet/defi-filters
branch
from
0d89160
to
29cdf6f
Compare
olivdb
force-pushed
the
minimal-wallet/defi-filters
branch
from
09ce94d
to
c19710a
Compare
olivdb
changed the title
olivdb
changed the title
olivdb
changed the title
| return (method == ERC20_APPROVE); | ||
| } | ||
|
|
||
| function getMethod(bytes memory _data) internal pure returns (bytes4 method) { |
There was a problem hiding this comment.
Maybe we should move this method to an abstract BaseFilter contract given that it will be used by all filters.
There was a problem hiding this comment.
There's already Utils.functionPrefix that does that
There was a problem hiding this comment.
Utils.functionPrefix has a require(_data.length >= 4, "Utils: Invalid functionPrefix"); which we don't want (we want to return false in most of these cases, and sometimes even accept _data.length == 0).
There was a problem hiding this comment.
But by the point where this is called you've already ensured that require is met
There was a problem hiding this comment.
True. But that's a useless require. Also, I like that we have tried so far to write filters in a way that doesn't rely on any other file than IFilter and BaseFilter, making it easier to publish the filters and DappRegistry in a standalone repo, should this become a reference registry used outside Argent.
olivdb
changed the title
test/paraswap.js
Outdated
|
|
||
| filter = await Filter.new(tokenPriceRegistry.address, dexRegistry.address); | ||
| await dappRegistry.addDapp(0, paraswap.address, filter.address); | ||
| await dappRegistry.addDapp(0, paraswapProxy, ZERO_ADDRESS); |
There was a problem hiding this comment.
Shouldn't we have a filter for the proxy as well?
| const AaveV2Filter = artifacts.require("AaveV2Filter"); | ||
| const BalancerFilter = artifacts.require("BalancerFilter"); | ||
| const YearnFilter = artifacts.require("YearnFilter"); | ||
|
|
There was a problem hiding this comment.
Missing the paraswap proxy filter
olivdb
changed the title
Includes implementation & tests for the following authorisation filters: