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

Making contracts `memory-safe` #711

Closed remedcu closed 8 months ago

remedcu commented 9 months ago

This PR does the following things:

Closes #544

github-actions[bot] commented 9 months ago

CLA Assistant Lite bot All contributors have signed the CLA ✍️ βœ…

remedcu commented 9 months ago

I have read the CLA Document and I hereby sign the CLA

coveralls commented 9 months ago

Pull Request Test Coverage Report for Build 7475596867


Totals Coverage Status
Change from base Build 6969581763: 0.0%
Covered Lines: 397
Relevant Lines: 405

πŸ’› - Coveralls
nlordell commented 9 months ago

No - I don’t think we want to migrate to v0.8 for the Safe contracts.

remedcu commented 9 months ago

I will make the necessary changes to go back to 0.7.6 πŸ‘πŸΎ

nlordell commented 9 months ago

One thing that I think this PR should do, is to go over our existing "memory-safe" assembly and remove unnecessary allocations like in:

https://github.com/safe-global/safe-contracts/blob/f03dfae65fd1d085224b00a10755c509a4eaacfe/contracts/base/FallbackManager.sol#L72-L75

For reference and some more context: https://github.com/safe-global/safe-core-protocol-specs/pull/67

mmv08 commented 8 months ago

Do you have an idea why this job failed? Also, it's interesting that it exited with 0 code.

https://github.com/safe-global/safe-contracts/actions/runs/7300399750/job/19894950822?pr=711

remedcu commented 8 months ago

Nice catch! Could be because of this: https://github.com/safe-global/safe-contracts/blob/optimizer-enabled/.github/workflows/ci.yml#L74

If you turn viaIR: false, it should work.

An issue was created for this in the backlog: https://github.com/orgs/safe-global/projects/14/views/10?sliceBy%5Bvalue%5D=Protocol&pane=issue&itemId=48283099

P.S. This benchmark has been failing for quite some time now that I checked. The last history I could find in the main was this one: https://github.com/safe-global/safe-contracts/actions/runs/6456132696/job/17524992449 (which is a 4-month-old merge to main branch)

remedcu commented 8 months ago

Do you have an idea why this job failed?

It is passing now @mmv08 https://github.com/safe-global/safe-contracts/actions/runs/7422997985/job/20199462482?pr=711

remedcu commented 8 months ago

Will make a new issue & PR once this is merged for the returndatasize. Will merge this one, once all the checks pass.

Thank you all for reviewing this PR!