Add validation for enum cases during contract updates #762
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
turbolent
approved these changes
Apr 5, 2021
runtime/errors.go
Outdated
Comment on lines
393
to
395
| declName string | ||
| expected int | ||
| found int |
There was a problem hiding this comment.
All other errors export the fields so maybe do the same here
Comment on lines
+1514
to
+1519
| fmt.Sprintf( | ||
| "mismatching enum case: expected `%s`, found `%s`", | ||
| expectedEnumCase, | ||
| foundEnumCase, | ||
| ), | ||
| enumMismatchError.Error(), |
There was a problem hiding this comment.
Maybe instead of comparing error messages literally, just check declName, expectedEnumCase, and foundEnumCase of the error directly, so changing the error message doesn't break the test
There was a problem hiding this comment.
Also wanted to assert the error message, to make sure they are consistent, and the to-string works with nested errors, etc.
Comment on lines
1527
to
1536
| assert.Equal( | ||
| t, | ||
| fmt.Sprintf( | ||
| "missing cases in enum `%s`: expected %d or more, found %d", | ||
| declName, | ||
| expectedCaes, | ||
| foundCases, | ||
| ), | ||
| extraFieldError.Error(), | ||
| ) |
There was a problem hiding this comment.
Maybe instead of comparing error messages literally, just check declName, expectedCases, and foundCases of the error directly, so changing the error message doesn't break the test
SupunS
force-pushed
the
supun/enum-update-validation
branch
from
b938b28
to
51bb585
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #757
Description
As in #757, the enum-case values are represented using a composite value, having a field called
rawValuewhich stores the enum-case raw value. Hence serializing/deserializing, only the raw value gets serialized/deserialized, but not the actual enum-case name. So changing enum-cases can lead to type/value confusions.This PR adds a validation to avoid such scenarios. With these changes, update to an enum need to make sure that:
The above restrictions would mean:
For contributor use:
masterbranchFiles changedin the Github PR explorer