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

Use core-contracts templates that use Environment #5401

Merged
merged 19 commits into from
Feb 27, 2024

Conversation

joshuahannan
Copy link
Member

@joshuahannan joshuahannan commented Feb 15, 2024

Uses the changes in this PR to make the getter functions for the core contracts less brittle by using lib/go/templates.Environment for the arguments instead of individual addresses

Uses the latest version of the core-contracts that keeps the LockedTokens.LockedAccountInfo interface

Also updates state commitments

Copy link
Contributor

@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.
Great change, thank you!

Base automatically changed from bastian/cadence-1-migration-fixes to feature/stable-cadence February 26, 2024 19:21
go.mod Outdated Show resolved Hide resolved
@turbolent
Copy link
Member

Pushed the merge in 666a58f, but TestCadenceValuesMigration fails:

  • ee82856bf20e2aa6.FungibleToken:

    Parsing failed:
    error: missing digits
      --> :34:26
       |
    34 | import ViewResolver from 0x
       |                           ^
    
  • 0ae53cb6e3f42a79.FlowToken:

    Parsing failed:
    error: unexpected token: identifier
     --> :3:7
      |
    3 | import FungibleTokenMetadataViews from
      |        ^
    

@joshuahannan
Copy link
Member Author

I'll pull the branch down and fix the issue

@codecov-commenter
Copy link

codecov-commenter commented Feb 26, 2024

Codecov Report

Attention: Patch coverage is 84.56376% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 56.37%. Comparing base (97775ab) to head (d6ae039).

Files Patch % Lines
cmd/util/cmd/epochs/cmd/deploy.go 0.00% 7 Missing ⚠️
...edger/migrations/change_contract_code_migration.go 78.12% 7 Missing ⚠️
fvm/systemcontracts/system_contracts.go 0.00% 7 Missing ⚠️
utils/unittest/execution_state.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@                    Coverage Diff                     @@
##           feature/stable-cadence    #5401      +/-   ##
==========================================================
- Coverage                   56.37%   56.37%   -0.01%     
==========================================================
  Files                        1032     1032              
  Lines                      100711   100648      -63     
==========================================================
- Hits                        56775    56737      -38     
+ Misses                      39628    39603      -25     
  Partials                     4308     4308              
Flag Coverage Δ
unittests 56.37% <84.56%> (-0.01%) ⬇️

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
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.

Looks good! Thanks for cleaning this up 👍

@turbolent turbolent requested review from a team and removed request for yhassanzadeh13 February 27, 2024 00:18
@turbolent
Copy link
Member

I think the integration tests need an updated Emulator, because that one is still using old contracts, e.g. TestWithEmulator errors with import RandomBeaconHistory from 0xRANDOMBEACONHISTORYADDRESS

@joshuahannan
Copy link
Member Author

Do you mean like a version that we currently have? Okay to use v1.0.0-M7?

@turbolent
Copy link
Member

@joshuahannan That version doesn't have the updated imports yet, does it?

I'll open a PR and update this one

@turbolent
Copy link
Member

I opened onflow/flow-emulator#582, but even there the tests importing the random beacon contract fail: https://github.com/onflow/flow-emulator/actions/runs/8058034335/job/22010193181?pr=582

@joshuahannan it looks like some imports still need to be updated from the old 0xCONTRACT to the new "Contract" import scheme

@turbolent
Copy link
Member

Fixed by onflow/flow-emulator@2ec5974

@joshuahannan
Copy link
Member Author

Let me know if you need anything else!

@turbolent
Copy link
Member

@joshuahannan Did you get a chance to look at unifying the versions that are used for the contracts and templates modules?

@turbolent
Copy link
Member

For some reason the integration test TestLoadTypes/token-transfer fails. I think what happens is that some import that used to be 0xCONTRACT is now "Contract", and then isn't replaced properly with an address, leaving the import as "Contract", which is a string location instead of an address location.

@turbolent
Copy link
Member

@joshuahannan You updated tokenTransferTransaction.cdc, but also need to update TokenTransferTransaction

@joshuahannan
Copy link
Member Author

joshuahannan commented Feb 27, 2024

I think I found one of them. I'm fixing. yep, it is TokenTransferTransaction

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!

@turbolent turbolent merged commit ffb80c9 into feature/stable-cadence Feb 27, 2024
50 of 51 checks passed
@turbolent turbolent deleted the josh/use-env-for-contracts branch February 27, 2024 20:47
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.

4 participants