-
Notifications
You must be signed in to change notification settings - Fork 5
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
Update ERC-6538 contract and tests #5
Conversation
@garyghayrat Can you request me as a reviewer so I remember to review this? I think with this comment you now should be able to |
Hmm, still don't see you in the reviewers list |
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.
Hey @garyghayrat, overall this is looking way better! I did leave some questions, the most important of which is related to the type hash. Thanks!
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 your patience @garyghayrat. More comments. But we are getting there.
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.
More comments @garyghayrat, I promise we're getting close and I'm not trying to drive you crazy 🙈
* Test internal variable initialization in registry constructor tests * Improvements to domain separator test * Improvements to the RegisteryKeys tests * Naming improvements to registerKeysOnBehalf tests * Update test imports and remove duplicated import * Refactor signature test helpers for test simplicity * Simplify mock contract for ERC1271 signatures * Remove redundant test and add missing ones for register on behalf * Minor cleanup to increment nonce test * Update the EIP712Domain separator to match the spec * Simplify Announcer test names * Use `vm.expectRevert("")` instead of `vm.expectRevert()`
471d3be
to
df75e3e
Compare
Coverage after merging erc-6538-use-address-only into main will be
Coverage Report
|
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.
Let's gooooooo 🚀
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
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 great!
No description provided.