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

client: add new shanghai engine apis #2509

Merged
merged 17 commits into from
Jan 30, 2023
Merged

client: add new shanghai engine apis #2509

merged 17 commits into from
Jan 30, 2023

Conversation

acolytec3
Copy link
Contributor

@acolytec3 acolytec3 commented Jan 25, 2023

Adds new engine apis

@codecov
Copy link

codecov bot commented Jan 25, 2023

Codecov Report

Merging #2509 (4e92d7e) into master (d986218) will increase coverage by 0.01%.
The diff coverage is 97.41%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 90.10% <ø> (ø)
blockchain 90.02% <ø> (ø)
client 87.59% <97.41%> (+0.08%) ⬆️
common 95.82% <ø> (ø)
devp2p 91.59% <ø> (-0.03%) ⬇️
ethash ∅ <ø> (∅)
evm 83.35% <ø> (ø)
rlp ∅ <ø> (∅)
statemanager 89.61% <ø> (ø)
trie 90.36% <ø> (-0.30%) ⬇️
tx 93.71% <ø> (ø)
util 84.52% <ø> (ø)
vm 83.68% <ø> (ø)

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

@acolytec3 acolytec3 changed the title add getCapabilities engine endpoint add new shanghai engine apis Jan 25, 2023
Comment on lines 1047 to 1049
for (const txn of block.transactions) {
transactions.push('0x' + txn.serialize().toString('hex'))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (const txn of block.transactions) {
transactions.push('0x' + txn.serialize().toString('hex'))
}
const transactions = block.transactions.map((tx) => bufferToHex(tx.serialize())) ?? []

Comment on lines 1050 to 1055
let withdrawals
if (block._common.gteHardfork(Hardfork.Shanghai)) {
withdrawals = []
for (const withdrawal of block.withdrawals!) {
withdrawals.push(withdrawal.toJSON() as WithdrawalV1)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let withdrawals
if (block._common.gteHardfork(Hardfork.Shanghai)) {
withdrawals = []
for (const withdrawal of block.withdrawals!) {
withdrawals.push(withdrawal.toJSON() as WithdrawalV1)
}
const withdrawals = block.withdrawals?.map(wt=>wt.toJSON())??null

we shouldn't be needing to explicity check for Shanghai as block construction would have taken care of it

message: 'Start and Count parameters cannot be less than 1',
}
}
const currentChainHeight = (await this.chain.getCanonicalHeadHeader()).number
Copy link
Contributor

Choose a reason for hiding this comment

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

we should be able to use chain.latest.height or something since its build similarly, could avoid wait/load from db

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Switched to this.chain.height.

for (const block of blocks) {
try {
const transactions: string[] = []
for (const txn of block.transactions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

similar simplification for transactions and withdrawals

@g11tech g11tech changed the title add new shanghai engine apis client: add new shanghai engine apis Jan 30, 2023
Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm!

@g11tech g11tech merged commit 02ec892 into master Jan 30, 2023
@holgerd77 holgerd77 deleted the add-new-engine-apis branch February 6, 2023 12:19
@holgerd77
Copy link
Member

Nice. 👍🙂

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

Successfully merging this pull request may close these issues.

3 participants