Make eth_getStorageAt spec-compliant

[fvictorio]

According to the spec, the storage slot in eth_getStorageAt should have a length of 66 (0x + 32 bytes).

While this is technically a bug fix, it will probably break things for many people. There are two solutions:

• To use helpers.getStorageAt, since the network helpers will be released along with this change (or soon enough).
• To use ethers.utils.hexZeroPad(storageSlot, 32) around the value to make it valid

[linear]

HH-321 The slot parameter of `eth_getStorageAt` should accept hex encoded unsigned integers

We are expecting the storage slot parameter of getStorageAt to be a QUANTITY, but the spec says that the parameter should be:

title  hex encoded unsigned integer
type  string
pattern  ^0x[0-9a-f]{64}$

Notice that the value should have a length of 64, but we'll reject many of those (because values with leading zeros are not a valid QUANTITY)

I don't know if we can start rejecting shorter values though. On one hand, we try to be as spec-compliant as possible, but in the other hand that would be a breaking change.

[changeset-bot]

🦋 Changeset detected

Latest commit: 83914cb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
hardhat	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[aathan]

This fix does not resolve the issue I'm having in Foundry, which is related to failed eth_getStorageAt calls in hardhat vs direct full-node RPC. I installed this branch and ran the problem test and I get the following. I also reported this to Foundry at foundry-rs/foundry#1297

eth_getStorageAt

  Errors encountered in param 1: Invalid value "0x0000000000000000000000000000000000000000000000000000000000000003" supplied to : QUANTITY

[aathan]

I think maybe the problem is from the calls to numberToRpcQuantity() in the eth_getStorageAt implementation itself, in getStorageAt.ts?

Nevermind I did:

git checkout origin/test-helpers
npm i --save-dev @types/node
git merge 99cd298a0d2d63edd797e2cb82cf9b73a010c57a
git merge 7338b79727c575ca9b4e630c1973e40d5001ef23
git merge d31bbe76f540959fbb8dfd41e07f1f8865832412
npm run build
cd other_project
rm -rf node_modules packa*
npm link ../hardhat/packages/*
npx hardhat node --network hardhat

and was able to verify that the fixes in this PR resolve the eth_getStorageAt issue I was facing from Foundry/forge tests.

[feuGeneA]

Left a suggestion and a question, this LGTM without necessarily resolving those.

[feuGeneA]

Do these bytestrings have any specific relevance? How does the expected output relate to these inputs?

[fvictorio]

Just a random storage slot, and an address that is (almost surely) empty. Added some comments explaining this.

[alcuadrado]

Just left a few comments, but LGTM

[fvictorio]

(Just a reminder for myself: we are not merging this until the network helpers are released)

[fvictorio]

This is now released in hardhat@2.9.5. This change means that the eth_getStorageAt method is harder to use because the storage slot has to be fully padded. As a better alternative to this, you can use our network helpers package (beta), that makes this kind of interaction easier.

[mattsse]

According to the spec, the storage slot in eth_getStorageAt should have a length of 66 (0x + 32 bytes).

this is a bit confusing to me because this spec https://ethereum.org/en/developers/docs/apis/json-rpc/#eth_getstorageat says it's a QUANTITY...

what am I missing?

[alcuadrado]

Hey @mattsse!

We are trying to follow the OpenRPC-based spec, but it can have mistakes. When in doubt we try to copy whatever Geth does tbh. We want to avoid users thinking that what they are doing should work, just to realize it doesn't when going into production and switching to Geth.

Note that in eth_getStorageAt the spec uses an uint256, and not an uint (the equivalent of QUANTITY). (example with uint instead)

This was also surprising to me, as before that spec we also used that description of the JSON-RPC as our first-line source of truth, so I just re-checked it.

In Geth it uses a hash type, which is 32 bytes. I guess that when they were writing this spec of this method uint256 was introduced to differentiate it from an actual hash.

[mattsse]

I see, this is clear now, thank you!

[aathan]

Hi guys, I've been following this thread since back in April when I reported a related issue to both the foundry and hardhat projects (see my comments on this bug in April). As a result I've dug into all of the history and my conclusion is that I believe there may be an error being made here on hardhat's side regarding the OpenRPC spec. Specifically:

#2230

contains a reference to a spec that states:

title  hex encoded unsigned integer
type  string
pattern  ^0x[0-9a-f]{64}$

However, the origin of that spec is unknown and, it seems NOT to be what's currently in the open rpc specification. In fact, the spec referenced at the opening of #2581 (this bug, yes, scroll to top) says:

title
hex encoded unsigned integer

pattern
^0x[0-9a-f]{0,64}$

(and by the way, @alcuadrado this is the same spec that appears in the spec you link to in your most recent comment)

I also chased down the regex specification format used by open rpc, which is perl's: From https://perldoc.perl.org/perlre we have {n,m} Match at least n but not more than m times

The issue of the hash vs uint vs uint256 type in the implementation seems distinct from what SHOULD be accepted by the RPC encoding specification. There is no source provided for the spec quoted in #2230, so it's not clear whether that is somehow more recent, but given that the current open rpc spec says {0,64} that seems doubtful.

Note: That being said {0,64} also seems wrong, as this would allow 0x as valid. {1,64} seems to be what was intended there.

[aathan]

I looked at geth and it does not appear to enforce a length on hash types.

In api.go:

func (s *BlockChainAPI) GetStorageAt(ctx context.Context, address common.Address, key string, blockNrOrHash rpc.BlockNumberOrHash) (hexutil.Bytes, error) {
	state, _, err := s.b.StateAndHeaderByNumberOrHash(ctx, blockNrOrHash)
	if state == nil || err != nil {
		return nil, err
	}
	res := state.GetState(address, common.HexToHash(key))
	return res[:], state.Error()
}

which eventually calls:

func BytesToHash(b []byte) Hash {
	var h Hash
	h.SetBytes(b)
	return h
}

which is implemented like this:

// SetBytes sets the hash to the value of b.
// If b is larger than len(h), b will be cropped from the left.
func (h *Hash) SetBytes(b []byte) {
	if len(b) > len(h) {
		b = b[len(b)-HashLength:]
	}

	copy(h[HashLength-len(b):], b)
}

[alcuadrado]

wow, thanks for digging deeper into this! This situation has been a whole mess and I unfortunately collaborated to it. You are right, and geth's implementation doesn't validate anything. Just look at this, wtf