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.

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

Revert "bump nim-web3 to enforce JSON-RPC Quantity syntax" #3850

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

tersec
Copy link
Contributor

@tersec tersec commented Jul 7, 2022

Reverts #3846

Correct as far as it goes, but
https://github.com/status-im/nim-web3/blob/ae12813602b55d022d99d06d3cabc8f9425d62d6/web3/ethtypes.nim#L90-L111
and some other things in ethtypes define certain fields as QUANTITY which are not, such as the nonce field of BlockObject (return type of getBlockBy...).

Revert for now, until another pull of a more complete transformation of nim-web3 ethtypes corrects relevant types.

@tersec tersec enabled auto-merge (squash) July 7, 2022 21:10
@tersec
Copy link
Contributor Author

tersec commented Jul 7, 2022

https://ethereum.org/en/developers/docs/apis/json-rpc/#eth_getblockbyhash and https://ethereum.org/en/developers/docs/apis/json-rpc/#eth_getblockbynumber lists/reference a

hash: DATA, 32 Bytes - hash of the block. null when its pending block.
as one of its field.

Nimbus currently does implement such a field in its BlockObject in ethtypes as a BlockHash.

The related (but potentially distinct) https://github.com/ethereum/execution-apis/blob/main/src/engine/specification.md#executionpayloadv1 lists

  • parentHash: DATA, 32 Bytes
  • blockHash: DATA, 32 Bytes

https://github.com/ethereum/execution-apis/blob/main/src/eth/block.yaml presents itself, and that GitHub repo is linked from ethereum.org/docs, as the authoritative source, and according to it, both eth_getBlockByHash and eth_getBlockByNumber return

    schema:
      $ref: '#/components/schemas/Block'

Following that link: https://github.com/ethereum/execution-apis/blob/main/src/schemas/block.yaml defines what it explicitly entitles a "Block object". This "Block object" that's returned by eth_getBlockByNumber and eth_getBlockByHash does not contain any defined hash field.

@tersec
Copy link
Contributor Author

tersec commented Jul 7, 2022

Part 2: difficulty and totalDifficulty.

https://ethereum.org/en/developers/docs/apis/json-rpc/#eth_getblockbyhash (which also defines getBlockByNumber's return type) lists them as

difficulty: QUANTITY - integer of the difficulty for this block.
totalDifficulty: QUANTITY - integer of the total difficulty of the chain until this block.

They're both QUANTITYs, and should presumably both be the same type (or at least, not differ in more than their size -- the specs allow for differently-sized numeric types, but QUANTITYs comply with https://ethereum.org/en/developers/docs/apis/json-rpc/#quantities-encoding

When encoding quantities (integers, numbers): encode as hex, prefix with "0x", the most compact representation (slight exception: zero should be represented as "0x0").

https://github.com/ethereum/execution-apis/blob/main/src/schemas/block.yaml states that

    difficulty:
      title: Difficulty
      $ref: '#/components/schemas/bytes'
    totalDifficulty:
      title: Total difficult
      $ref: '#/components/schemas/uint'

First, they're not the same:

	"bytes": {
		"title": "hex encoded bytes",
		"type": "string",
		"pattern": "^0x[0-9a-f]*$"
	},

	"uint": {
		"title": "hex encoded unsigned integer",
		"type": "string",
		"pattern": "^0x([1-9a-f]+[0-9a-f]*|0)$"
	},

bytes allows leading zeros and uint is basically this QUANTITY type. This is a real difference.

Furthermore, difficulty in blocks.yaml allows for leading 0s and in the ethereum.org/docs spec does not.

@tersec
Copy link
Contributor Author

tersec commented Jul 7, 2022

Part 3, unformatted data/DATA:

https://ethereum.org/en/developers/docs/apis/json-rpc/#unformatted-data-encoding states that

When encoding unformatted data (byte arrays, account addresses, hashes, bytecode arrays): encode as hex, prefix with "0x", two hex digits per byte.

in particular, "two hex digits per byte". This appears to be the requirement placed on all of the DATA return fields of https://ethereum.org/en/developers/docs/apis/json-rpc/#eth_getblockbyhash / https://ethereum.org/en/developers/docs/apis/json-rpc/#eth_getblockbynumber: hash [if it exists; see part 1], parentHash, nonce, sha3Uncles, logsBloom, etc. Thus, it appears to prohibit specifying a byte with half-zeros and notes that:

WRONG: 0xf0f0f (must be even number of digits)
is invalid.

These fields in https://github.com/ethereum/execution-apis/blob/main/src/schemas/block.yaml are specified as one of:

      $ref: '#/components/schemas/hash32'
      $ref: '#/components/schemas/address'
      $ref: '#/components/schemas/bytes256'
      $ref: '#/components/schemas/bytes'
      $ref: '#/components/schemas/bytes8'

with reference to https://github.com/ethereum/execution-apis/blob/main/src/schemas/base-types.yaml which defines their regexes fine for fixed-length types, e.g.,

address:
  title: hex encoded address
  type: string
  pattern: ^0x[0-9,a-f,A-F]{40}$

for a 20-byte binary blob, which implies the two hex digits per byte, but for variable-length types,

bytes:
  title: hex encoded bytes
  type: string
  pattern: ^0x[0-9a-f]*$

allows for, say, 0xf0f0f, because it does not constrain the length to be even, with two hex digits per byte.

@tersec tersec merged commit 91b51ad into unstable Jul 8, 2022
@tersec tersec deleted the revert-3846-DhX branch July 8, 2022 00:07
arnetheduck added a commit that referenced this pull request Sep 5, 2022
* bearssl: use master branch again
* chronos: less Option, better contenttype handling
* eth, presto: less imports
* stew: typo
* web3: tighter `Quantity` parsing rules (see
#3850 and subsequently
status-im/nim-web3#55)
arnetheduck added a commit that referenced this pull request Sep 5, 2022
* bearssl: use master branch again
* chronos: less Option, better contenttype handling
* eth, presto: less imports
* stew: typo
* web3: tighter `Quantity` parsing rules (see
#3850 and subsequently
status-im/nim-web3#55)
@arnetheduck arnetheduck mentioned this pull request Sep 5, 2022
arnetheduck added a commit that referenced this pull request Sep 6, 2022
* bearssl: use master branch again
* chronos: less Option, better contenttype handling
* eth, presto: less imports
* stew: typo
* web3: tighter `Quantity` parsing rules (see
#3850 and subsequently
status-im/nim-web3#55)
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.

None yet

1 participant