-
Notifications
You must be signed in to change notification settings - Fork 213
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: implement the ERC2612 (Permit) extension for StETH #486
Conversation
0f443dc
to
85ee91c
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.
Isn't it better to move the whole IERC2612 implementation to the separate 0.8.9 subcontract. It will allow to use more OpenZeppelin code without any modification.
It will be more concise and clear, and substantially less code to review and less calls to external contract per permit.
Am I missing something?
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.
🔥
I guess it's time to merge before the mass-review begins. LGTM to merge, but needs a deeper and wider look in the future.
NB. No spec and tests yet.
7e6ce20
to
4015478
Compare
@folkyatina please have a look and decide whether we can put forward this one to the trunk or polish it up 👍 |
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.
LGTM. But there is some points to think about.
const digest = web3.utils.keccak256( | ||
'0x1901' + | ||
strip0x(domainSeparator) + | ||
strip0x(web3.utils.keccak256(web3.eth.abi.encodeParameters(['bytes32', ...types], [typeHash, ...parameters]))) |
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.
Thinking about converging to using ethers.js library for Etherium everywhere in tests. Just a topic to think about, not a requirement.
Resolves #440.