-
Notifications
You must be signed in to change notification settings - Fork 66
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
Extract type requirements from old code for staged contracts #1705
Conversation
@@ -187,10 +190,12 @@ func (v *stagingValidatorImpl) Validate(stagedContracts []stagedContractUpdate) | |||
|
|||
v.stagedContracts = make(map[common.AddressLocation]stagedContractUpdate) | |||
for _, stagedContract := range stagedContracts { | |||
v.stagedContracts[stagedContract.DeployLocation] = stagedContract | |||
stagedContractLocation := stagedContract.DeployLocation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contract update validator relies on typeIDs. So had to use the DeployLocation
in all places, instead of SourceLocation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also needed to fix https://discord.com/channels/613813861610684416/1277718471618330707/1277750460723363911
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a dedicated test for ^above: bd87b9f
7c5d6c1
to
68d9ad5
Compare
68d9ad5
to
091c693
Compare
typeRequirements := &migrations.LegacyTypeRequirements{} | ||
|
||
// Extract type requirements from the old codes for all staged contracts. | ||
for _, contract := range v.stagedContracts { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actual improvement is this block: extract type requirements from old code
// If the contract one of our staged contract updates, use the source location | ||
if stagedUpdate, ok := v.stagedContracts[resovledAddrLocation]; ok { | ||
resolvedLocation = stagedUpdate.SourceLocation | ||
} else { | ||
resolvedLocation = resovledAddrLocation | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is no need to separate the staged contracts vs other contracts now, given staged contracts also uses DeployLocation
, which is an address location.
This PR/issue depends on: |
…n/support-type-requirements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
@jribbink can you PTAL?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1705 +/- ##
==========================================
- Coverage 34.32% 34.15% -0.18%
==========================================
Files 100 100
Lines 6368 6386 +18
==========================================
- Hits 2186 2181 -5
- Misses 3894 3922 +28
+ Partials 288 283 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Work towards supporting onflow/cadence#3527 in Flow CLI.
i.e: integrating onflow/flow-go#6341 to the CLI.
Description
Depends on onflow/flow-go#6395
For contributor use:
master
branchFiles changed
in the Github PR explorer