-
Notifications
You must be signed in to change notification settings - Fork 71
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
Integration with Solday's ERC6551 Implementation #133
Conversation
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, needs @Vectorized's review
For tests, it'd be better to use parameterized internal functions for repetitive code, e.g. |
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 good
Description
Overview
This PR introduces integration with Solday's ERC6551 smart contract. Key modifications include the update of the
state
variable, renaming it fromnonce
and changing its type fromuint256
tohash (bytes32)
. This adjustment ensures thatstate
is now deterministic, computed from the input parameters of theexecute()
function.Modifications
state
fromuint256
tobytes32
according to Solady 6551, which is hash computed based onexecute()
inputs. All execute functionsexecute()',
executeBatch()and
executeWithSig()share the function to update 'state' which calculate hash based on common inputs of execute functions:
to,
valueand
data`.execute()
andexecuteBatch()
methods from Solady ERC6551 to include check permissions via AccessController. This ensures that every execution undergoes a permission verification process.Test Plan
All new added code covered by automation tests.
Related Issue
Closes #83