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

Added EIP-4844 Headers for BlockHeaderOutput. Issue: 6933 #6937

Merged
merged 6 commits into from
Apr 15, 2024

Conversation

ymonye
Copy link
Contributor

@ymonye ymonye commented Mar 26, 2024

Added EIP-4844 & EIP-4788 Headers for the BlockHeaderOutput, following Go-Ethereum headers:
https://github.com/ethereum/go-ethereum/blob/738b5a586e329965539877434b695bb61015d4c7/core/types/block.go#L65

getBlock() now uses blockHeaderSchema instead of blockSchema.

Original Issue: #6933

Description

Please include a summary of the changes and be sure to follow our Contribution Guidelines.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • [*] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • [*] I have selected the correct base branch.
  • [*] I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • [*] My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • [*] I ran npm run lint with success and extended the tests and types if necessary.
  • [*] I ran npm run test:unit with success.
  • I ran npm run test:coverage and my test cases cover all the lines and branches of the added code.
  • I ran npm run build and tested dist/web3.min.js in a browser.
  • [*] I have tested my code on the live network.
  • I have checked the Deploy Preview and it looks correct.
  • I have updated the CHANGELOG.md file in the root folder.
  • I have linked Issue(s) with this PR in "Linked Issues" menu.

…es blockHeaderSchema instead of blockSchema.
@luu-alex
Copy link
Contributor

luu-alex commented Mar 26, 2024

Thanks for this, could u remove the yarn.lock and update the changelog for web3-eth and web3-types
Could u give PR edit access to contributors of this project as well, just in case i need to update this pr

@@ -257,7 +258,7 @@ export async function getBlock<ReturnFormat extends DataFormat>(
hydrated,
);
}
return format(blockSchema, response as unknown as Block, returnFormat);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you would want to edit blockSchema and add the new properties there, not editing blockHeaderSchema and just using blockSchema

Copy link
Contributor Author

@ymonye ymonye Mar 26, 2024

Choose a reason for hiding this comment

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

I believe you would want to edit blockSchema and add the new properties there, not editing blockHeaderSchema and just using blockSchema

Ok, yeah I'd noticed both the withdrawals & withdrawalsRoot properties only appear under blockHeaderSchema, in the schemas.ts file. When I call getBlock() on web3.js v1.9, it outputs both of those properties. This is why I'd assumed getBlock() probably needed blockHeaderSchema defined within v4's rpc_method_wrappers.ts file instead of blockSchema, and thought it was a typo.

Should blockSchema & blockHeaderSchema have identical properties then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea its a bit confusing, It seems like blockSchema needs to also have updated properties of withdrawals and withdrawalsRoot. blockHeaderSchema is only being used for subscriptions. We may need to update the blockHeaderSchema as well, if you want to check and confirm result of a subscription the result you can also edit blockheaderSchema, otherwise you can focus on just updating blockSchema

Copy link
Contributor Author

@ymonye ymonye Mar 27, 2024

Choose a reason for hiding this comment

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

Ok, I've updated both blockSchema & blockHeaderSchema in schemas.ts, with blockHeaderSchema retaining the below author & excessDataGas properties while missing from blockSchema. Please let me know if blockSchema should also have these.

blockSchema & blockHeaderSchema also have different nested arrays for the transactions property, and were left as originally found.

author: {
   format: 'bytes32',
},
excessDataGas: {
   format: 'uint',
},

The BlockOutput & BlockHeaderOutput interfaces in eth_types.ts were also updated accordingly.

Under eth_types.ts, I'd noticed a number of properties are formatted as either bigint | number or Numbers, whether they are defined under BlockOutput or BlockHeaderOutput.

For example: totalDifficulty in BlockOutput:
readonly totalDifficulty?: bigint | number;

Whereas in BlockHeaderOutput, totalDifficulty is formatted as:
readonly totalDifficulty?: Numbers;

I'm unsure if it should be one or the other, but I've left as-is. Also the extra properties: author & excessDataGas were not added to the BlockOutput interface, as they are not present in blockSchema. However I did add everything else from BlockHeaderOutput interface into BlockOutput, while formatting the ones with Numbers as bigint | number. Please advise if that was the correct move.

Also, let me know if those author & excessDataGase properites should also be present across blockSchema/blockHeaderSchema & BlockOutput/BlockHeaderOutput.

yarn.lock had its changes reverted, as well as rpc_method_wrappers.ts.

Copy link
Contributor Author

@ymonye ymonye Mar 27, 2024

Choose a reason for hiding this comment

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

Also, it appears the 'newHeads' & 'newBlockHeaders' subscriptions only return formatted data from the BlockBase interface in eth_types.ts, and not from the BlockHeaderOutput interface.

https://docs.web3js.org/api/web3-eth/class/NewHeadsSubscription

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a note here so I don't forget after sleep. Looks like more parameters are missing from v4.6 yet present in v1.9, outside the scope of EIP-4844 changes, specifically across other eth_* functions. I'll do a much proper rundown thru the next couple of days, likely into the weekend.

For ex: the id parameter is missing in the logs array, from eth_getTransactionReceipt. Example below from an ETH mainnet request:

web3 v4.6:
{"address":"0xb584d4be1a5470ca1a8778e9b86c81e165204599","blockHash":"0x47c58ebbaaff9a9a9ee60d74029224df7cac63b5c09972d4306e7500d8078b55","blockNumber":"19529894","data":"0x0000000000000000000000000000000000000000000000000000022ecb25c000","logIndex":"469","removed":false,"topics":["0x90890809c654f11d6e72a28fa60149770a0d11ec6c92319d6ceb2bb0a4ea1a15","0x0000000000000000000000000a25e45792fef9fb600a81f0d7ac4ab9e893fc71","0x0000000000000000000000000000000000000000000000000000000000000064"],"transactionHash":"0xe2bad713807549fe3bb0bf0a69dce0ddc634c8dbec655660ea038279ef5bdb91","transactionIndex":"170"}

web3 v1.9:
{"address":"0xb584D4bE1A5470CA1a8778E9B86c81e165204599","blockHash":"0x47c58ebbaaff9a9a9ee60d74029224df7cac63b5c09972d4306e7500d8078b55","blockNumber":19529894,"data":"0x0000000000000000000000000000000000000000000000000000022ecb25c000","id":"log_ff3f9ddd","logIndex":469,"removed":false,"topics":["0x90890809c654f11d6e72a28fa60149770a0d11ec6c92319d6ceb2bb0a4ea1a15","0x0000000000000000000000000a25e45792fef9fb600a81f0d7ac4ab9e893fc71","0x0000000000000000000000000000000000000000000000000000000000000064"],"transactionHash":"0xe2bad713807549fe3bb0bf0a69dce0ddc634c8dbec655660ea038279ef5bdb91","transactionIndex":170}

@ymonye
Copy link
Contributor Author

ymonye commented Mar 27, 2024

Thanks for this, could u remove the yarn.lock and update the changelog for web3-eth and web3-types Could u give PR edit access to contributors of this project as well, just in case i need to update this pr

I think I did this, gave you access to my forked repo

Copy link
Contributor

@jdevcs jdevcs left a comment

Choose a reason for hiding this comment

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

remove changes in yarn.lock,

@ymonye
Copy link
Contributor Author

ymonye commented Mar 27, 2024

remove changes in yarn.lock,

Done

Copy link

codecov bot commented Mar 28, 2024

Codecov Report

Merging #6937 (8fdfa11) into 4.x (c4e039a) will increase coverage by 0.00%.
Report is 2 commits behind head on 4.x.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##              4.x    #6937   +/-   ##
=======================================
  Coverage   91.95%   91.96%           
=======================================
  Files         215      215           
  Lines        8188     8197    +9     
  Branches     2208     2214    +6     
=======================================
+ Hits         7529     7538    +9     
  Misses        659      659           
Flag Coverage Δ
UnitTests 91.96% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
web3 ∅ <ø> (∅)
web3-core ∅ <ø> (∅)
web3-errors ∅ <ø> (∅)
web3-eth ∅ <ø> (∅)
web3-eth-abi ∅ <ø> (∅)
web3-eth-accounts ∅ <ø> (∅)
web3-eth-contract ∅ <ø> (∅)
web3-eth-ens ∅ <ø> (∅)
web3-eth-iban ∅ <ø> (∅)
web3-eth-personal ∅ <ø> (∅)
web3-net ∅ <ø> (∅)
web3-providers-http ∅ <ø> (∅)
web3-providers-ipc ∅ <ø> (∅)
web3-providers-ws ∅ <ø> (∅)
web3-rpc-methods ∅ <ø> (∅)
web3-utils ∅ <ø> (∅)
web3-validator ∅ <ø> (∅)

@luu-alex
Copy link
Contributor

sorry for the long awaited response, looks good :) this will be included in the next release. Thanks for your efforts!

},
logsBloom: {
format: 'bytes256',
blobGasUsed: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +299 to +322
parentBeaconBlockRoot: {
format: 'bytes32',
},
parentHash: {
format: 'bytes32',
},
receiptsRoot: {
format: 'bytes32',
},
sha3Uncles: {
format: 'bytes32',
},
size: {
format: 'uint',
},
stateRoot: {
format: 'bytes32',
},
timestamp: {
format: 'uint',
},
totalDifficulty: {
format: 'uint',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

these fields are already in current schema but this PR is only rearranging these to diff lines.

Comment on lines +172 to +202
readonly baseFeePerGas?: Numbers;
readonly blobGasUsed?: Numbers;
readonly difficulty?: Numbers;
readonly excessBlobGas?: Numbers;
readonly extraData?: Bytes;
readonly gasLimit: Numbers;
readonly gasUsed: Numbers;
readonly hash?: HexString32Bytes;
readonly logsBloom?: Bytes;
readonly miner?: HexString;
readonly nonce?: Numbers;
readonly number?: Numbers;
readonly parentBeaconBlockRoot?: HexString32Bytes;
readonly parentHash?: HexString32Bytes;
readonly receiptsRoot?: HexString32Bytes;
readonly sha3Uncles: HexString32Bytes[];
readonly stateRoot?: HexString32Bytes;
readonly timestamp: Numbers;
readonly transactionsRoot?: HexString32Bytes;
readonly withdrawalsRoot?: HexString32Bytes;

// These fields are returned when the RPC client is Nethermind,
// but aren't available in other clients such as Geth
readonly author?: Address;
readonly totalDifficulty?: Numbers;
readonly size?: Numbers;
readonly excessDataGas?: Numbers;
readonly mixHash?: HexString32Bytes;
readonly transactions?: TransactionOutput[];
readonly uncles?: Uncles;
readonly withdrawals?: Withdrawals[];
Copy link
Contributor

Choose a reason for hiding this comment

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

this is also rearranging most of field except adding ( eip 4844, 4895, 4788 )s

Copy link
Contributor

@jdevcs jdevcs left a comment

Choose a reason for hiding this comment

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

lgtm, @luu-alex plus add changelog in release PR for this:
added support of eip4844, eip4895, eip4788 block fields

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.

4 participants