Skip to content

Conversation

@frangio
Copy link
Contributor

@frangio frangio commented Sep 20, 2022

Fixes #438
Fixes #477

User defined value types are not supported without storage layout info, this is fine because they only exist in >=0.8.8, and I think we always have storage layout info for those versions. Unsure about Truffle to be honest, do we have storage layout in Truffle?

@frangio
Copy link
Contributor Author

frangio commented Sep 20, 2022

I added tests for when we don't have detailed layout info (specifically numberOfBytes). I don't think we can currently support that on Truffle, any upgrade involving UDVTs will result in an error. The way that I would like us to support that is to check that original and upgraded contracts were compiled with >= 0.8.9 (because of #438), but we don't currently have the version information along with the layout. This would appear to be further complicated by the fact that some layouts are already stored in the manifest and we can't update them retroactively with compiler version information. But a contract with UDVTs would not be deployable or importable with the plugins until this change, so AFAICT there shouldn't be any manifest out there with UDVTs. Ironically this means that we need to start including compiler version information in this PR if we want to have a chance to support Truffle in the future.

@KholdStare
Copy link

KholdStare commented Sep 21, 2022

Hi @frangio. I just tested out this particular commit on my local codebase, and now I am getting a different error further along, still to do with User Defined Value Types. This time it looks like it's doing something in getFunctionSignature.

Error: No node with id 10052 of type StructDefinition,EnumDefinition,ContractDefinition
    at deref (/home/kholdstare/workspace/code/crypto/evm/node_modules/@openzeppelin/upgrades-core/src/ast-dereferencer.ts:38:11)
    at curried (/home/kholdstare/workspace/code/crypto/evm/node_modules/@openzeppelin/upgrades-core/src/utils/curry.ts:13:14)
    at serializeTypeName (/home/kholdstare/workspace/code/crypto/evm/node_modules/@openzeppelin/upgrades-core/src/utils/function.ts:22:31)
    at /home/kholdstare/workspace/code/crypto/evm/node_modules/@openzeppelin/upgrades-core/src/utils/function.ts:52:72
    at Array.map (<anonymous>)
    at getFunctionSignature (/home/kholdstare/workspace/code/crypto/evm/node_modules/@openzeppelin/upgrades-core/src/utils/function.ts:52:55)
    at /home/kholdstare/workspace/code/crypto/evm/node_modules/@openzeppelin/upgrades-core/src/validate/run.ts:173:45
    at Array.map (<anonymous>)
    at validate (/home/kholdstare/workspace/code/crypto/evm/node_modules/@openzeppelin/upgrades-core/src/validate/run.ts:173:12)
    at OverriddenTaskDefinition._action (/home/kholdstare/workspace/code/crypto/evm/node_modules/@openzeppelin/hardhat-upgrades/src/index.ts:81:25)

I can help out to narrow down a minimal example contract, but I don't know how to find out what node 10052 is referencing in the ast. Are there any artifacts I can look at or generate that will tell me what node 10052 is, and what contract it's in?

@frangio
Copy link
Contributor Author

frangio commented Sep 21, 2022

Thanks for testing. The ids are present in the AST in artifacts/build-info files. I was able to reproduce the issue though.

@KholdStare
Copy link

@frangio I tested on the commit in this PR (e75232f), and it no longer crashes! 🎉

Copy link
Member

@ericglau ericglau left a comment

Choose a reason for hiding this comment

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

LGTM!

@ericglau
Copy link
Member

Can you also add a changelog entry?

@frangio
Copy link
Contributor Author

frangio commented Sep 23, 2022

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support user defined value types. Detect storage incompatibilites due to bug in Solidity 0.8.8

4 participants