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

Add Change Contract Code Migration #5191

Merged
merged 15 commits into from
Jan 18, 2024

Conversation

janezpodhostnik
Copy link
Contributor

@janezpodhostnik janezpodhostnik commented Jan 2, 2024

@janezpodhostnik janezpodhostnik self-assigned this Jan 2, 2024
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice!

I'll see which contracts need to be updated and add them. This will need to support all networks

@turbolent
Copy link
Member

turbolent commented Jan 5, 2024

I guess we can use SystemContractsForChain to get the names and addresses for each chain.

Currently it returns:

  • Epoch-related contracts: FlowEpoch, FlowIDTableStaking, FlowClusterQC, FlowDKG
  • Service account-related contracts: FlowServiceAccount, NodeVersionBeacon, RandomBeaconHistory, FlowStorageFees
  • Token-related contracts: FlowFees, FlowToken, FungibleToken
  • NFT-related contracts: NonFungibleToken, MetadataViews, ViewResolver
  • EVM

Are all of them deployed at bootstrapping time and need to be updated by the migration?

Are there any other contracts that need to be updated using this migration?

cc @joshuahannan

@joshuahannan
Copy link
Member

More contracts that I can think of that might be worth upgrading as part of this migration:

Tokens: FungibleTokenMetadataViews, FungibleTokenSwitchboard, FiatToken (USDC)
Marketplace: NFTStorefront, NFTStorefrontV2
Hybrid Custody: basically everything in this directory: https://github.com/onflow/hybrid-custody/tree/main/contracts
Core Contracts: LockedTokens, StakingProxy, FlowStakingCollection

These are all pretty important, but none of them are critical for core network operation as far as I know, so I'm not sure if they all need to belong in this migration, but I could probably make arguments for most of them.

@turbolent
Copy link
Member

@joshuahannan Thanks! Can you please review e7e4a4f and check the values are correct (addresses, imports, code)?

It adds FungibleTokenMetadataViews, FungibleTokenSwitchboard, and LockedTokens, StakingProxy, FlowStakingCollection; for the remaining contracts the code is not easily available

Base automatically changed from janez/deduplicate-contract-names to master January 9, 2024 14:50
@joshuahannan
Copy link
Member

Looks good to me! We'll be starting on the other contracts soon

@turbolent turbolent marked this pull request as ready for review January 10, 2024 00:09
@turbolent turbolent requested review from a team January 10, 2024 00:09
@turbolent
Copy link
Member

@janezpodhostnik Can you please have a look at my changes? (cannot request a review from you, as you opened the PR)

@codecov-commenter
Copy link

codecov-commenter commented Jan 11, 2024

Codecov Report

Attention: 185 lines in your changes are missing coverage. Please review.

Comparison is base (3c27e97) 55.99% compared to head (c272728) 56.21%.

Files Patch % Lines
...edger/migrations/change_contract_code_migration.go 28.01% 183 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5191      +/-   ##
==========================================
+ Coverage   55.99%   56.21%   +0.21%     
==========================================
  Files         649      995     +346     
  Lines       63379    95260   +31881     
==========================================
+ Hits        35492    53551   +18059     
- Misses      25321    37728   +12407     
- Partials     2566     3981    +1415     
Flag Coverage Δ
unittests 56.21% <28.01%> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor Author

@janezpodhostnik janezpodhostnik left a comment

Choose a reason for hiding this comment

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

Looks good besides one comment.

@turbolent turbolent enabled auto-merge January 18, 2024 20:48
@turbolent turbolent added this pull request to the merge queue Jan 18, 2024
Merged via the queue into master with commit 5fe4f98 Jan 18, 2024
50 of 51 checks passed
@turbolent turbolent deleted the janez/change-contract-code-migration branch January 18, 2024 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants