Skip to content
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

Add MultiQuarkOperation functions to QuarkWalletProxyFactory #187

Merged
merged 3 commits into from
Mar 22, 2024

Conversation

kevincheng96
Copy link
Collaborator

Adds createAndExecuteMulti functions to the QuarkWalletProxyFactory.

Copy link
Contributor

@hayesgm hayesgm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the functions look good, not sure why the EIP-712 domain separator is replicated

@@ -19,6 +19,15 @@ contract QuarkWalletProxyFactory {
/// @notice Default initial salt value
bytes32 public constant DEFAULT_SALT = bytes32(0);

/// @notice The EIP-712 domain separator for a MultiQuarkOperation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why this is here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided some time ago to put the domain separator in the factory as well. IIRC, it was a gas optimization to move the public getters for domain separators from the wallet to the factory. Now that we are using proxy wallets, it doesn't really matter. But having these be on the factory is still useful in the case where the wallet does not yet exist.

Base automatically changed from kevin/multiquarkoperation to main March 22, 2024 18:55
@kevincheng96 kevincheng96 merged commit bb166ca into main Mar 22, 2024
4 checks passed
@kevincheng96 kevincheng96 deleted the kevin/multi-factory branch March 22, 2024 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants