-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add erc20 token limit module #184
Conversation
Summary by OctaneNew Contracts
Updated ContractsNo contracts were updated in this PR. 🔗 Commit Hash: e4bce76 |
Contract sizes: | Contract | Size (B) | Margin (B) |
|------------------------------|----------|------------|
| AccountFactory | 4,169 | 20,407 |
+| ERC20TokenLimitModule | 4,138 | 20,438 |
| ModularAccount | 25,983 | -1,407 |
| SemiModularAccount | 26,487 | -1,911 |
| SingleSignerValidationModule | 3,444 | 21,132 |
| TokenReceiverModule | 2,189 | 22,387 | Code coverage:
|
This comment was marked as spam.
This comment was marked as spam.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! Had some comments.
ad9f9cf
to
67d2af7
Compare
6e41348
to
f0af33b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Had just small comment and unused import nits.
import {UserOperationLib} from "@eth-infinitism/account-abstraction/core/UserOperationLib.sol"; | ||
import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; | ||
|
||
import {SetValue} from "@erc6900/modular-account-libs/libraries/AssociatedLinkedListSetLib.sol"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the refactor, this import and the _toSetValue
internal function are no longer used.
|
||
import {SetValue} from "@erc6900/modular-account-libs/libraries/AssociatedLinkedListSetLib.sol"; | ||
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; | ||
import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The EnumerableSet import and the using EnumerableSet for EnumerableSet.AddressSet;
statement are both unused now and can be removed. I think the linter sees the using
statement and thinks it is used, so it misses this case.
return ""; | ||
} | ||
|
||
/// @inheritdoc IModule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the natspec comment requests for other modules, could you add the encoding format of data
to natpspec as an @dev
comment? This should help by making it show up in docs and on explorers.
f0af33b
to
dcc3ac4
Compare
dcc3ac4
to
0d54c95
Compare
Motivation
Account holders may want to set ERC20 token spend limit to certain validations.
Solution
Create an ERC20 token limit module to offer hooks associated with validations that works with both UO and runtime call paths. See code doc for features and restrictions.