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.

Sign up for GitHub

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

Jump to bottom

fix: proxy deploy with storage #3235

Merged
merged 7 commits into from
Oct 2, 2024
Merged

fix: proxy deploy with storage #3235

merged 7 commits into from
Oct 2, 2024

Conversation

danielbate
Copy link
Contributor

@danielbate danielbate commented Oct 1, 2024 •
edited
Loading

Release notes

In this release, we:

  • Fixed contracts containing storage deployed with a proxy via fuels deploy

Summary

When a contract with storage is deployed via proxy, the proxy must be aware of the target contracts storage slots.

Checklist

  • All changes are covered by tests (or not applicable)
  • All changes are documented (or not applicable)
  • I reviewed the entire PR myself (preferably, on GH UI)
  • I described all Breaking Changes (or there's none)

@vercel Vercel
Copy link

vercel bot commented Oct 1, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fuels-template ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 2, 2024 9:48am
ts-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 2, 2024 9:48am
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
create-fuels-counter-example ⬜️ Ignored (Inspect) Oct 2, 2024 9:48am

@github-actions github-actions bot added the bug Issue is a bug label Oct 1, 2024
arboleya
arboleya reviewed Oct 1, 2024
View reviewed changes
Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

Mostly nits.

packages/fuels/src/cli/commands/deploy/deployContract.ts Outdated Show resolved Hide resolved
packages/fuels/src/cli/commands/deploy/deployContract.ts Outdated Show resolved Hide resolved
packages/fuels/src/cli/commands/deploy/deployContract.ts Outdated Show resolved Hide resolved
packages/fuels/test/features/deploy.test.ts Outdated Show resolved Hide resolved
packages/fuels/test/features/deploy.test.ts Outdated Show resolved Hide resolved
arboleya
arboleya previously approved these changes Oct 1, 2024
View reviewed changes
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 1, 2024

Coverage Report:

Lines Branches Functions Statements
76.65%(-0.02%) 71.45%(-0.03%) 75.54%(-0.11%) 76.75%(-0.01%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 packages/fuels/src/cli/commands/deploy/deployContract.ts 100%
(+0%)		 70%
(-13.33%)		 100%
(+0%)		 100%
(+0%)
🔴 packages/fuels/src/cli/commands/deploy/proxy/types/Src14OwnedProxyFactory.ts 33.33%
(-33.33%)		 0%
(+0%)		 0%
(-66.66%)		 33.33%
(-33.33%)

petertonysmith94
petertonysmith94 reviewed Oct 1, 2024
View reviewed changes
Copy link
Contributor

@petertonysmith94 petertonysmith94 left a comment

Choose a reason for hiding this comment

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

Happy to write up the tests about storage slots. Otherwise looks good.

packages/fuels/test/features/deploy.test.ts Show resolved Hide resolved
packages/fuels/test/features/deploy.test.ts Show resolved Hide resolved
packages/fuels/src/cli/commands/deploy/deployContract.ts Show resolved Hide resolved
packages/fuels/test/features/deploy.test.ts Show resolved Hide resolved
@danielbate
Copy link
Contributor Author

@petertonysmith94 as mentioned in the conversations, we do not handle custom storage slots in fuels deploy, same as forc deploy. This should be implemented manually.

This PR ensures the storage for the target contract is initialised on the proxy contract. IMO a test for the function using storage on the target contract suffices.

petertonysmith94
petertonysmith94 previously approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@petertonysmith94 petertonysmith94 left a comment

Choose a reason for hiding this comment

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

@danielbate Apologies for my lack of understanding behind this functionality.

Thanks for clarifying.

Torres-ssf
Torres-ssf previously approved these changes Oct 1, 2024
View reviewed changes
@danielbate danielbate changed the base branch from master to next October 2, 2024 09:43
@danielbate danielbate dismissed stale reviews from Torres-ssf and petertonysmith94 October 2, 2024 09:43

The base branch was changed.

@danielbate danielbate dismissed arboleya’s stale review October 2, 2024 09:43

The base branch was changed.

@danielbate danielbate marked this pull request as ready for review October 2, 2024 09:44
@danielbate danielbate enabled auto-merge (squash) October 2, 2024 10:53
@danielbate danielbate self-assigned this Oct 2, 2024
@danielbate danielbate merged commit 83b0ea9 into next Oct 2, 2024
24 checks passed
@danielbate danielbate deleted the db/fix/fuels-deploy-proxy-storage branch October 2, 2024 20:52
@fuel-service-user fuel-service-user mentioned this pull request Oct 4, 2024
Merged
Dhaiwat10 pushed a commit that referenced this pull request Nov 19, 2024
@danielbate @Dhaiwat10
fix: proxy deploy with storage (#3235)
b2879ac 
* test: add assertions for the storage methods

* feat: pass target slots to proxy contract

* chore: nits

* chore: cleanup test

* chore: lint

* chore: changeset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arboleya arboleya arboleya approved these changes

@Torres-ssf Torres-ssf Torres-ssf approved these changes

@maschad maschad maschad approved these changes

@digorithm digorithm Awaiting requested review from digorithm

@Dhaiwat10 Dhaiwat10 Awaiting requested review from Dhaiwat10

@nedsalk nedsalk Awaiting requested review from nedsalk

@petertonysmith94 petertonysmith94 Awaiting requested review from petertonysmith94

Assignees

@danielbate danielbate

Labels
bug Issue is a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Contracts with storage deployed via proxy with fuels deploy are broken
5 participants
@danielbate @arboleya @maschad @petertonysmith94 @Torres-ssf