Refactor erc20 around the _spendAllowance function #240
Conversation
|
This should be good for review |
| func name_() -> (name: felt): | ||
| end |
There was a problem hiding this comment.
| func name_() -> (name: felt): | |
| end | |
| func ERC20_name() -> (name: felt): | |
| end |
We should still prefix all storage variables to prevent clashes with other libs. Another benefit with namespace though is we can omit the ugly proceeding underscore.
There was a problem hiding this comment.
IMO storage should not be imported. Contracts using libraries like this one should only rely on functions (including getters and setters) that are in the corresponding namespace.
Would clash happen for storage with the same name that are part of the dependencies but that are not explicitly imported?
There was a problem hiding this comment.
IMO storage should not be imported. Contracts using libraries like this one should only rely on functions (including getters and setters) that are in the corresponding namespace.
This is probably the safer/better design play. I can agree here.
Would clash happen for storage with the same name that are part of the dependencies but that are not explicitly imported?
Yes with the caveat that the compiler will throw an AssertionError if the same-name storage vars differ in any way—including return value name, variable name, and number of keys. Otherwise, the same-name storage vars will have the same storage address and seen/treated as the same storage.
There was a problem hiding this comment.
Would clash happen for storage with the same name that are part of the dependencies but that are not explicitly imported?
Yes with the caveat that the compiler will throw an AssertionError if the same-name storage vars differ in any way—including return value name, variable name, and number of keys. Otherwise, the same-name storage vars will have the same storage address and seen/treated as the same storage.
IMO, this is terrible language design ...
I renamed the storage slots to account for that.
|
|
||
| func constructor{ | ||
| syscall_ptr : felt*, | ||
| pedersen_ptr : HashBuiltin*, |
There was a problem hiding this comment.
Since constructor is not protected and does not clash, my only concern is adding confusion to decorated versus undecorated constructors. Overall, I think this works better though—especially with seeing how it looks in the preset contracts. I'm curious what @martriay thinks.
There was a problem hiding this comment.
No strong opinions, although not sure if it counts as a constructor since it's not initializing a contract but a subset logic. Maybe initializer helps people distinguish between these two but again, no strong opinions.
There was a problem hiding this comment.
Excellent work on the changes! This looks good to me. I think @martriay should take a look as well.
| await signer.send_transaction(account, erc20.contract_address, 'approve', [spender.contract_address, *MAX_UINT256]) | ||
|
|
||
| # check approval | ||
| execution_info_1 = await erc20.allowance(account.contract_address, spender.contract_address).call() |
There was a problem hiding this comment.
I'm not very fond of execution_info_n for these kind of things but I don't have a better suggestion yet
| end | ||
|
|
||
| let (current_allowance: Uint256) = ERC20_allowances.read(owner, spender) | ||
| let (infinite: Uint256) = uint256_not(Uint256(0, 0)) |
There was a problem hiding this comment.
MAX_UINT256 would be a great addition to utils/constants.cairo
Fixes #238