-
Notifications
You must be signed in to change notification settings - Fork 194
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
Challenge 6 extension #249
Conversation
} | ||
|
||
function recover(bytes32 _hash, bytes memory _signature) public pure returns (address) { | ||
return ECDSA.recover(MessageHashUtils.toEthSignedMessageHash(_hash), _signature); |
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.
This line is changed because of openZeppelin
v5. See previous version here
contractName: "MetaMultiSigWallet", | ||
}); | ||
|
||
const historyHashes = useMemo(() => eventsHistory?.map(ev => ev.args.hash) || [], [eventsHistory]); |
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.
also changed this fromev.log.args.hash
to ev.args.hash
since event history hook was rewritten
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.
GJ Rinat! It's looking good to me 🙏
Only issue I found is events look duplicated, but haven't found a pattern. My first events looked duplicated, but then (after 2-3 extra transactions signed by the mulsitig) the first events look fine and the last events look duplicated.
Can try to check later if it was happening also in the old ch6 or if it comes from the changes in our hook.
Good catch Pablo, thanks!
It works in the old ch6 as expected, no need to check. Looks like we need to update |
I fixed it inside SE-2. Let's wait a bit until the fix is backmerged to create-eth, recheck if everything works as expected for this extension, and then merge this |
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 @damianmarti ! Updated PR Also, waiting for scaffold-eth/create-eth#172 to fix events on multisig page |
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.
@rin-st Thanks for the fixes!
Tested with latest In the future we might need to update all the challenges to start using the new hook parameter spec. |
Yes, thank you! Merging this! |
To test
Fixes #248