Check Signatures moved to Safe.sol

safe-global / safe-smart-account

Safe allows secure management of blockchain assets.

https://safe.global

GNU Lesser General Public License v3.0

1.84k stars 907 forks source link

Check Signatures moved to Safe.sol #721

Closed remedcu closed 8 months ago

remedcu commented 9 months ago

TODO:

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

Closes #707

coveralls commented 9 months ago

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
Change from base Build 6969581763:	-0.02%
Covered Lines:	395
Relevant Lines:	403

💛 - Coveralls

mmv08 commented 8 months ago

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

mmv08 commented 8 months ago

For example, I wonder if it will be possible to fix this bug if this function is returned: https://github.com/safe-global/safe-contracts/issues/708. I prefer new features if we have to choose between gas savings for 3rd parties or new features.

remedcu commented 8 months ago

The bytecode size increased by around 400 for this PR. More info: https://github.com/safe-global/safe-contracts/issues/707#issuecomment-1822480178

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

mmv08 commented 8 months ago

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 commented 8 months ago

So, around 50 bytes higher, right? I think I can solve that with the solution I am implementing for https://github.com/safe-global/safe-contracts/issues/713 So, it should not be a problem, but could be a blocker until I make a PR for that issue.

mmv08 commented 8 months ago

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.