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

Check Signatures moved to Safe.sol #721

Merged
merged 4 commits into from
Jan 4, 2024
Merged

Check Signatures moved to Safe.sol #721

merged 4 commits into from
Jan 4, 2024

Conversation

remedcu
Copy link
Member

@remedcu remedcu commented Dec 26, 2023

TODO:

  • Moving checkSignatures(bytes32,bytes,bytes) to Safe.sol
  • Changing the CompatibilityFallbackHandler contract to use checkSignatures(...) from Safe contract.
  • Extra: Added fmt script for formatting both sol and ts files together.

Closes #707

@remedcu remedcu self-assigned this Dec 26, 2023
@coveralls
Copy link

coveralls commented Dec 26, 2023

Pull Request Test Coverage Report for Build 7330010573

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 94.551%

Totals Coverage Status
Change from base Build 6969581763: -0.02%
Covered Lines: 395
Relevant Lines: 403

💛 - Coveralls

@remedcu remedcu requested a review from akshay-ap December 28, 2023 10:50
@remedcu remedcu merged commit 0acdd35 into main Jan 4, 2024
20 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 4, 2024
@mmv08
Copy link
Member

mmv08 commented Jan 4, 2024

What are the bytecode size implications of this? IMO this PR was merged too early.

@mmv08
Copy link
Member

mmv08 commented Jan 4, 2024

For example, I wonder if it will be possible to fix this bug if this function is returned: #708. I prefer new features if we have to choose between gas savings for 3rd parties or new features.

@remedcu
Copy link
Member Author

remedcu commented Jan 4, 2024

The bytecode size increased by around 400 for this PR. More info: #707 (comment)

How much size increase is estimated for #708 PoC solution?

@mmv08
Copy link
Member

mmv08 commented Jan 4, 2024

The bytecode size increased by around 400 for this PR. More info: #707 (comment)

How much size increase is estimated for #708 PoC solution?

With the draft solution in https://github.com/safe-global/safe-contracts/tree/bug/execTransactionFromModuleReturnData-mit-guard the SafeL2 is at 24222 bytes.

EDIT: have just pulled from main, and it's above the limit with the checkSignature change. SafeL2 24625 bytes (limit is 24576).

@remedcu
Copy link
Member Author

remedcu commented Jan 4, 2024

So, around 50 bytes higher, right? I think I can solve that with the solution I am implementing for #713 So, it should not be a problem, but could be a blocker until I make a PR for that issue.

@mmv08
Copy link
Member

mmv08 commented Jan 4, 2024

So, around 50 bytes higher, right? I think I can solve that with the solution I am implementing for #713 So, it should not be a problem, but could be a blocker until I make a PR for that issue.

Sounds good. I'm still curious whether bringing back an outdated function at the cost of being borderline to the bytecode limit is a good idea. That means that if we ever need to add a change that results in an increased bytecode size, be that a new feature or a bug fix, we might not be able to do that.

@nlordell nlordell deleted the checkSignatures branch April 22, 2024 09:19
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.

[Safe 1.5.0] Evaluate Posibility of moving checkSignatures(bytes32, bytes, bytes) into Safe
4 participants