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

Send encoded Safe operation bytes to checkSignatures call #165

Merged
merged 4 commits into from
Nov 22, 2023

Conversation

nlordell
Copy link
Collaborator

Fixes #160

This PR fixes the checkSignatures call to the Safe to include both the Safe operation hash for recovery, but also the encoded operation bytes, which is required when using an ERC-1271 signer. In order to test this, the SafeMock now requires that dataHash == keccak256(data) and I added a unit test using a nested Safe set (note that this would require a staked paymaster to work with 4337 bundlers, but the test does check that the contract signatures works as expected).

@nlordell nlordell requested a review from a team as a code owner November 22, 2023 09:55
@nlordell nlordell requested review from rmeissner, akshay-ap, mmv08 and remedcu and removed request for a team November 22, 2023 09:55
@coveralls
Copy link

coveralls commented Nov 22, 2023

Pull Request Test Coverage Report for Build 6955856472

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 6901823068: 0.0%
Covered Lines: 29
Relevant Lines: 29

💛 - Coveralls

@@ -152,6 +155,75 @@ describe('Safe4337Module - Reference EntryPoint', () => {
expect(await ethers.provider.getBalance(safe.address)).to.be.eq(accountBalance - transfers - deposits)
})

it('should support a Safe signer (NOTE: would require a staked paymaster for ERC-4337)', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

is that because the signer would access its storage and that goes against the rules?

@mmv08
Copy link
Member

mmv08 commented Nov 22, 2023

Have just seen the public data commit. I think it's fine to leave it as internal, because it can be computed offchain

@nlordell
Copy link
Collaborator Author

I think it's fine to leave it as internal, because it can be computed offchain

Reverted. I was unsure about the change - but decided to propose it anyway just in case. The rationale was that it would be useful for computing SafeMessage hashes (where the bytes preimage is used instead of the bytes32 value).

@nlordell nlordell merged commit 7cfb9dd into master Nov 22, 2023
6 checks passed
@nlordell nlordell deleted the check-signatures-data branch November 22, 2023 10:20
@github-actions github-actions bot locked and limited conversation to collaborators Nov 22, 2023
@mmv08
Copy link
Member

mmv08 commented Nov 22, 2023

I think it's fine to leave it as internal, because it can be computed offchain

Reverted. I was unsure about the change - but decided to propose it anyway just in case. The rationale was that it would be useful for computing SafeMessage hashes (where the bytes preimage is used instead of the bytes32 value).

cant wait for 1.5.0 to get released 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[4337] Safe ERC-4337 Module Does not Support Contract Signatures
3 participants