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.

Sign up for GitHub

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

Jump to bottom

Validate contract updates #593

Merged
merged 15 commits into from
Feb 17, 2021
Merged

Validate contract updates #593

merged 15 commits into from
Feb 17, 2021

Conversation

SupunS
Copy link
Member

@SupunS SupunS commented Feb 10, 2021 •
edited
Loading

Work towards #588

Description

This PR introduces a validation step before updating a contract, to avoid any data inconsistencies.

✔️ This validate and prevents:

  • Changing the type of a contract field.
  • Adding a new contract field.
  • Changing type of a field / adding a field, of a nested declaration
  • Converting/updating a contract to a contract-interface, and vise-versa. Otherwise one can change from contract -> contract-interface -> contract, that may again cause field type incompatibility (i.e: indirect update to an incompatible state)
  • Doing any of above for contract-interfaces (may be we can relax this later?).

✔️ Things that are allowed:

  • Removing a field.
  • Updating composite types that are not directly/indirectly referred by a contract field.
  • changing functions (signature and content).

For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@SupunS SupunS changed the title Supun/validate contract updates Validate contract updates Feb 10, 2021
@SupunS SupunS force-pushed the supun/validate-contract-updates branch from 23e1d2e to d09944b Compare February 10, 2021 07:59
@SupunS SupunS marked this pull request as ready for review February 10, 2021 08:06
@SupunS SupunS requested a review from turbolent as a code owner February 10, 2021 08:06
@SupunS SupunS added the Feature label Feb 10, 2021
turbolent
turbolent reviewed Feb 10, 2021
View reviewed changes
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 work! Code looks overall great, just a few comments on re-using existing code and code organization.

runtime/contract_update_validation.go Outdated Show resolved Hide resolved
runtime/contract_update_validation.go Outdated Show resolved Hide resolved
runtime/contract_update_validation.go Outdated Show resolved Hide resolved
runtime/contract_update_validation.go Outdated Show resolved Hide resolved
runtime/contract_update_validation.go Outdated Show resolved Hide resolved
runtime/contract_update_validation.go Outdated Show resolved Hide resolved
runtime/runtime.go Outdated Show resolved Hide resolved
runtime/runtime.go Outdated
Comment on lines 1735 to 1737
if isUpdate {
validator := NewContractUpdateValidator(existingCode, program.Program)
err := validator.Validate()
Copy link
Member

Choose a reason for hiding this comment

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

👏

runtime/runtime_test.go Outdated Show resolved Hide resolved
runtime/runtime_test.go
@@ -6122,3 +6122,643 @@ func TestPanics(t *testing.T) {
)
assert.Error(t, err)
}

func TestContractUpdateValidation(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Great tests! 👏

@SupunS SupunS force-pushed the supun/validate-contract-updates branch 2 times, most recently from 11ccdd3 to 6b8414f Compare February 11, 2021 09:07
@SupunS SupunS force-pushed the supun/validate-contract-updates branch from 6b8414f to 4862f89 Compare February 11, 2021 09:38
@SupunS SupunS force-pushed the supun/validate-contract-updates branch 7 times, most recently from 95e208f to c814b79 Compare February 11, 2021 15:45
@SupunS SupunS force-pushed the supun/validate-contract-updates branch from c814b79 to 11dba10 Compare February 11, 2021 15:47
@SupunS
Copy link
Member Author

SupunS commented Feb 11, 2021 •
edited
Loading

@turbolent Thanks for the suggestions! Added the required changes.

@SupunS SupunS force-pushed the supun/validate-contract-updates branch from e5fb27f to a255035 Compare February 11, 2021 17:49
turbolent
turbolent reviewed Feb 13, 2021
View reviewed changes
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.

Great refactor! Just a couple last comments and after that it should be ready to go

runtime/contract_update_validation.go Outdated Show resolved Hide resolved
runtime/ast/declaration.go
@@ -26,4 +26,5 @@ type Declaration interface {
DeclarationIdentifier() *Identifier
DeclarationKind() common.DeclarationKind
DeclarationAccess() Access
DeclarationMembers() *Members
Copy link
Member

Choose a reason for hiding this comment

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

👍

runtime/contract_update_validation.go
oldFields := oldDeclaration.DeclarationMembers().FieldsByIdentifier()
newFields := newDeclaration.DeclarationMembers().Fields()

// Updated contract has to have at-most the same number of field as the old contract.
Copy link
Member

Choose a reason for hiding this comment

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

👍

runtime/contract_update_validation.go Outdated Show resolved Hide resolved
runtime/contract_update_validation.go Outdated Show resolved Hide resolved
runtime/contract_update_validation.go Outdated Show resolved Hide resolved
runtime/contract_update_validation.go Outdated Show resolved Hide resolved
turbolent
turbolent approved these changes Feb 16, 2021
View reviewed changes
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.

Very nice! 👏

@SupunS SupunS merged commit 37fad8c into master Feb 17, 2021
@SupunS SupunS deleted the supun/validate-contract-updates branch March 4, 2021 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turbolent turbolent turbolent approved these changes

Assignees

@SupunS SupunS

Labels
Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SupunS @turbolent