Add support for EIP-7201 namespaces

[frangio]

No description provided.

[socket-security]

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
hardhat-ignore-warnings	0.2.9	None	+14	3.76 MB	frangio
ethereum-cryptography	2.0.0	None	+4	2.3 MB	paulmillr

🚮 Removed packages: solidity-ast@0.4.49

[Amxx]

Starting to review. Here is a first round of comments.

[Amxx]

I'd love to understand this change.

My understanding so far is that if node id is not of type nodeType, we don't go in the if, and we implicitely return nothing (undefined) ?

Before the change, I guess resolveNode would fail and that would be catched by ASTDereferencerError ?

Can you explain why this change does / why its needed ?

[frangio]

To be honest this change ended up not being necessary, so we could revert it, but there is an explanation behind it.

What happened was that when I first introduced the namespace logic the transpiler slowed down significantly. To transpile OZ Contracts took perhaps 1 minute. So in debugging I traced it down to resolveContract/tryResolveNode and experimented with approaches to improve this.

The issue was that deref: ASTDereferencer, used by tryResolveNode, had an optimized path that worked only when the requested node type was correct. But tryResolveNode was being used through resolveContract to check if a node id corresponded to a ContractDefinition. If the id was instead something like a StructDefinition, deref would fall back to brute force enumeration of all nodes in the source code to try to find one with this id (!).

The reason I introduced the '*' feature in ASTDerefencer was to avoid falling back to the slow path, instead quickly returning whatever node was found even if it was not the wanted node type.

This helped somewhat but I ended up finding a much better optimization that causes this to not really matter. (That should also make a lot of other things faster, including hardhat-upgrades and hardhat-exposed.)

[Amxx]

I'm curious if there is a way to make the type explicit in something like

Suggested change

	const hre: HardhatRuntimeEnvironment = (await import(hardhat)).default;
	const { default: hre } = await import(hardhat);

[frangio]

Yes but it's more verbose. It would be like this:

  const { default: hre }: { default: HardhatRuntimeEnvironment } = await import(hardhat);

[Amxx]

do we want the __XXX_init and __XXX_init_unchained to be at the top like this ?

[frangio]

They are added at the top because there is no constructor in this case, but I suppose it would be nicer if this was added right before the first function.

[frangio]

Fixed in the latest commit.

[Amxx]

spacing is a bit strange.

[frangio]

Spacing here is inherited from the original code, where variables look like this:

    uint constant C = 3;
    // a
    uint x;
    // b
    /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
    uint immutable y = 2;
    uint private z;

    string private s1 = "";

So I think it's good!

[Amxx]

It really comes down to what // a and // b means and refer to. I find it strange that // a is attached to x, and // b is attached to y.

[Amxx]

that may not be an issue if variables are sorted (first constants, then immutable, then private)

[frangio]

My reasoning was that if a comment is immediately before a variable, it should be attached to that variable. But I didn't consider the case where there is a newline between comment and variable, in that case I would say they should be considered separate and the comment should possibly be left outside of the struct...

// atached comment
uint y;

// separate comment

uint x;

[frangio]

We're very limited in how we can handle comments because they're not present in the AST so I would leave the current behavior, unless it's producing results that we don't like for the Contracts codebase.

[frangio]

I realized none of the existing tests are passing the output through the compiler. We rely on OpenZeppelin Contracts tests for that. It makes sense to add compilation tests in this repo but we should do this in a follow up PR.

[HardlyDifficult]

@frangio could we make the slot getters internal virtual (e.g. function _getC3Storage() internal pure virtual returns (C3Storage storage $))? It may be possible for contracts already deployed to specify their existing storage location this way, allowing us to upgrade to the latest OZ. It would be risky of course, in case something else about the storage had changed, and would require a lot of testing - but may give us a path to stay on the latest OZ libraries.

[frangio]

It may be possible for contracts already deployed to specify their existing storage location this way

It's not possible, we've changed many things about the storage variables. We don't want to encourage this because it's guaranteed to break. For now, if you want to take this risk, feel free to fork the code and change the storage locations.