-
Notifications
You must be signed in to change notification settings - Fork 140
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
Allows adding new interface conformances during a contract update #1395
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1395 +/- ##
==========================================
+ Coverage 75.81% 75.84% +0.02%
==========================================
Files 279 279
Lines 38780 38774 -6
==========================================
+ Hits 29402 29407 +5
+ Misses 8016 8007 -9
+ Partials 1362 1360 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
494d614
to
41d78ec
Compare
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:master commit d3124c5 Results
|
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.
Great work!
for _, oldConformance := range oldConformances { | ||
found := false | ||
for _, newConformance := range newConformances { |
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.
Maybe we can optimize the quadratic search here to avoid blow-up (in a follow-up PR). I guess because the outer loop break on first missing, it is actually OK?
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.
Optimized it here: 3c22c34
|
||
err := deployAndUpdate(t, "Test1", oldCode, newCode) | ||
require.NoError(t, err) | ||
}) | ||
} | ||
|
||
func TestConformanceChanges(t *testing.T) { |
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.
Good idea to add full runtime tests 👍
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!
// Remove the matched conformance, so we don't have to check it again. | ||
// i.e: optimization | ||
newConformances = append(newConformances[:index], newConformances[index+1:]...) |
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.
Great idea! Double checked with https://go.dev/play/p/N233f-oGIDh, as I was not quite sure how modifying newConformances
here affects the range
over it / the inner loop
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.
It always breaks from the inner-loop after modification, so should be safe. Thanks for double-checking!
Work towards #1394
Description
This PR allows adding interface conformances during a contract update.
Removing existing conformances are continue to be not allowed.
master
branchFiles changed
in the Github PR explorer