Validate storage layout #92

spalladino · 2018-09-13T22:36:01Z

When running zos-push, validate that the storage layout is compatible with the previous version of the contract, and abort push otherwise. All warnings are displayed to the user, eg:

$ zos push
Validating contract AnotherImplV1 before push
Variable 'uint256 value22' in contract ImplV1 was renamed to foo in contracts/mocks/ImplV1.sol:1.
Note that the new variable foo will have the value of value22 after upgrading. If this is not the desired behavior, add a new variable foo at the end of your contract instead.
New variable 'uint256 bar' was added in contract ImplV1 in contracts/mocks/ImplV1.sol:1
This pushes all variables after bar to a higher position in storage, causing the updated contract to read incorrect initial values. Only add new variables at the end of your contract to prevent this issue.

Storage layout is extracted from the contract's AST by the Storage class in lib, and is stored along with other contract data (such as bytecode hash) in the zos.network.json file. It is then retrieved for comparison on the next zos push. The format is a tuple (storage, types), similar to the format discussed in ethereum/solidity#4017, where storage contains the array of variables, with references to the types dictionary with detailed info on each type.

Comparison is handled by the Layout class in lib, and uses a modified levenshtein algorithm for calculating the minimum change between both storages. Changes are identified as additions (either at the end or mid-contract, with additions at the end computed at zero cost for the algorithm), deletions (end or mid-contract), or substitutions (renames, typechanges, or both).

Fixes #37

Pending:

Handle changes in complex types (structs)
Adding more tests for Layout
Improve and refactor CLI output
Test CLI output

facuspagnuolo · 2018-09-17T09:16:27Z

Let's please create some issues for the pending items

facuspagnuolo

Amazing job! Congrats @spalladino 👏
Left really minor comments :)

facuspagnuolo · 2018-09-17T09:34:33Z

packages/cli/src/models/files/ZosNetworkFile.js

@@ -192,12 +192,13 @@ export default class ZosNetworkFile {
    this.setDependency(name, fn(this.getDependency(name)));
  }

-  addContract(alias, instance) {
+  addContract(alias, instance, extraData = {}) {


I like the idea of using object arguments when those are kind of irrelevant to the rest of the parameters and if they are some type of configuration for the function being called. In this case, note that that extraData has the same importance as the rest of the parameters, we are deconstructing it as is, and it is impossible for the reader to know what data is actually being handled here. It forces the reader to look up for calls to this function to understand how it should be called. It is definitely not a blocker, but it is one of the things we should decide and standardize for the whole codebase.

facuspagnuolo · 2018-09-17T09:38:40Z

packages/cli/src/models/network/NetworkBaseController.js

+    const [begin, count] = src.split(':');
+    return sourceCode.substr(begin, count);
+  }
+


I'd move all this logic outside the controller

Absolutely!

facuspagnuolo · 2018-09-17T09:44:24Z

packages/lib/src/validations/Layout.js

+      DELETION_COST = 2;
+
+export function compareStorageLayouts(original, updated) {
+  // TODO: Check cases with empty storage (both for original and updated)


let's move this TODO to an issue please

facuspagnuolo · 2018-09-17T09:45:23Z

packages/lib/src/validations/Layout.js

+}
+
+// Adapted from https://gist.github.com/andrei-m/982927 by Andrei Mackenzie
+function levenshtein(originalStorage, updatedStorage, areEqualFn) {


facuspagnuolo · 2018-09-17T09:45:49Z

packages/lib/src/validations/Layout.js

+  const originalType = originalTypes[originalVar.type],
+        updatedType = updatedTypes[updatedVar.type];
+
+  // TODO: Compare complex types (structs and enums)


same here, let's create another issue for that

facuspagnuolo · 2018-09-17T09:48:21Z

packages/lib/src/validations/Storage.js

+      ? this.getValueTypeInfo(node.valueType)
+      : this.getTypeInfo(node)
+  }  
+}


facuspagnuolo · 2018-09-17T09:49:34Z

packages/lib/src/validations/Storage.js

+      kind: 'elementary',
+      label: 'function' 
+    } 
+  }


could we move this one and the one above to constants?

spalladino · 2018-09-21T20:16:34Z

@facuspagnuolo I implemented the suggested changes, and added issues #112, #113, and #114 to cover for the missing pieces (all in Backlog for now). The only thing I'm wondering is whether we may want to add a feature toggle to disable these warnings until those issues are finished, though I think we can do that later if we find out that we can't get them ready by the next release.

spalladino added status:in-progress status:in-progress Under development, do not merge this PR and removed status:in-progress labels Sep 13, 2018

facuspagnuolo previously approved these changes Sep 17, 2018

View reviewed changes

This was referenced Sep 21, 2018

Check structs in storage layout validation #112

Open

Check enums in storage layout validation #113

Open

Add more test cases for storage layout checks #114

Open

spalladino dismissed facuspagnuolo’s stale review via 4098342 September 21, 2018 20:14

spalladino added 21 commits September 21, 2018 17:14

Generate storage and types info for a single contract

14327a4

Add glob as dependency

92f3bf4

Handle references to other files and inheritance chains

fe864ec

Add support for functions and repeated ast ids

8101f68

Support recursive structs

80446a3

Refactor tests for storage layout

1cfe1c0

Support bytes in storage layout

02c56e0

Add tests for references to contracts in dependencies

fb3aed7

Report changes in layout storage via edit distance

af4c25b

Provide info on substitution kind from Layout comparison

8926e27

Check storage layout compatibility on zos push

ca5a2ab

Add flag to force zos push ignoring validation errors

f9c7922

Differentiate between delete and pop in storage layout comparison

059c6cb

Remove commented out console log

fb5a2f1

Add src and path reference in storage layout info

a7618ba

Add variables indices in storage to layout comparison output

62cb71f

Log storage changes from CLI

57eb1d5

Fix commands tests to include push new parameter

7ab958d

Minor change to CLI output

8f1d813

Fix storage layout test

43073f2

Implement suggestions by facuspagnuolo

9d69946

spalladino force-pushed the feature/check-storage-layout branch from 4098342 to 9d69946 Compare September 21, 2018 20:14

spalladino closed this Sep 24, 2018

spalladino changed the base branch from next to master September 24, 2018 20:15

spalladino mentioned this pull request Sep 24, 2018

Validate storage layout #117

Merged

frangio removed the status:in-progress Under development, do not merge this PR label Jan 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate storage layout #92

Validate storage layout #92

spalladino commented Sep 13, 2018 •

edited

Loading

facuspagnuolo commented Sep 17, 2018

facuspagnuolo left a comment

facuspagnuolo Sep 17, 2018

facuspagnuolo Sep 17, 2018

spalladino Sep 17, 2018

facuspagnuolo Sep 17, 2018

facuspagnuolo Sep 17, 2018

facuspagnuolo Sep 17, 2018

facuspagnuolo Sep 17, 2018

facuspagnuolo Sep 17, 2018

spalladino commented Sep 21, 2018

Validate storage layout #92

Validate storage layout #92

Conversation

spalladino commented Sep 13, 2018 • edited Loading

facuspagnuolo commented Sep 17, 2018

facuspagnuolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalladino commented Sep 21, 2018

spalladino commented Sep 13, 2018 •

edited

Loading