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

GREEN-42 add gasUsed and transaction root for returned block data #15

Merged
merged 6 commits into from
Jun 6, 2024

Conversation

tonititi
Copy link
Contributor

Add block gasUsed and transaction root to the returned block data

Copy link

linear bot commented Apr 24, 2024

package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@mhanson-github mhanson-github left a comment

Choose a reason for hiding this comment

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

Remove ^ in version.

Address other issue with misspelled parameter.

Retest

package.json Outdated
@@ -36,6 +36,7 @@
"got": "11.8.6",
"jayson": "3.6.5",
"jsonwebtoken": "9.0.0",
"merkle-patricia-tree": "^4.2.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove carat in version.

// Now we have the block gas used then return it to the RPC method
resultBlock.gasUsed = blockGasUsed
// Start to calculate the transaction root fot this block
resultBlock.transactionsRoot = await this.calculateTransactionRoot(readableBlock.tranactions)
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to be misspelled. Am not sure how this could have worked.

afostr
afostr previously requested changes Apr 28, 2024
Copy link
Contributor

@afostr afostr left a comment

Choose a reason for hiding this comment

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

build does not compile
~/shardus/npm-release/json-rpc-server$ npm run compile

[email protected] compile
tsc -p .

src/external/Collector.ts:20:27 - error TS2307: Cannot find module 'ethers' or its corresponding type declarations.

20 import { BigNumber } from 'ethers'
~~~~~~~~

Found 1 error.

@shardeum shardeum deleted a comment from afostr Apr 28, 2024
@tonititi tonititi closed this May 3, 2024
@akirapham akirapham reopened this May 8, 2024

import { BaseTrie } from 'merkle-patricia-tree'
import RLP from 'rlp'
import { BigNumber } from 'ethers'
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the import lib has to be @ethersproject/bignumber

@@ -245,6 +247,19 @@ class Collector extends BaseExternal {
return await this.inner_getBlock(blockSearchValue, blockSearchType, details)
}

async calculateTransactionRoot(txns: String[]): Promise<String> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the correct type has to be string

@tonititi
Copy link
Contributor Author

By changing the command npm ci to npm i in both smoke tests 10+10 and 25+5, these two tests were passed

25+5: https://devs-work.shardeum.org/job/Testing/job/Smoke/job/work_in_progress/job/wip_green_25_plus_5/2/
10+10: https://devs-work.shardeum.org/job/Testing/job/Smoke/job/work_in_progress/job/wip_test_green_team/2/

jairajdev
jairajdev previously approved these changes May 31, 2024
@jairajdev jairajdev dismissed stale reviews from afostr and mhanson-github June 6, 2024 16:28

fixed

@jairajdev jairajdev force-pushed the GREEN-42-Fix-incorrect-returned-block-data branch from e003c85 to f220900 Compare June 6, 2024 16:35
@jairajdev jairajdev merged commit 704a6b2 into dev Jun 6, 2024
1 check passed
@mhanson-github mhanson-github deleted the GREEN-42-Fix-incorrect-returned-block-data branch September 7, 2024 05:36
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.

6 participants