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

Issue 173: Adding uncle count support #236

Closed
wants to merge 11 commits into from
Closed

Issue 173: Adding uncle count support #236

wants to merge 11 commits into from

Conversation

ccowell
Copy link
Contributor

@ccowell ccowell commented Nov 30, 2018

PR Summary

geth_api_double.js

  • Added RPC methods for get eth_getUncleCountBy*

block_helper.js

  • Set uncle array

forkedblockchain.js

  • Added uncle stats to a new forked block

compileAndDeploy.js, pretest_setup.js

  • Adding these files to each test as a refactoring effort until merged into develop

rpc.js

  • Starting to modularizing the rpc methods as part of the refactoring effort.

@ccowell ccowell requested a review from eshaben November 30, 2018 18:37
Copy link
Contributor

@eshaben eshaben left a comment

Choose a reason for hiding this comment

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

looks good to me!

: "../index.js");

describe("Uncle support", function() {
describe("retrieving uncles", function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the nested describe block necessary? Can we combine it to be one describe block? Or for readability and clarity, should it stay nested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be combined but went with the nested route for more readability when running npm test

describe("retrieving uncles", function() {
const services = preloadWeb3();

it("by hash", async function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anyway the it blocks throughout this file could be changed to be more readable like a sentence? For this particular one maybe it("should return result by hash") or something?

Copy link
Contributor Author

@ccowell ccowell Nov 30, 2018

Choose a reason for hiding this comment

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

Could definitely go that route but was attempting to keep it concise for quick referencing during an npm test run.

Sample output would look like

Uncle support
    retrieving uncle count
      - by hash
      - by block number
      - by block tag

const { gasLimit } = await web3.eth.getBlock("latest");
const instance = await contract.deploy({ data: bytecode }).send({ from: accounts[0], gas: gasLimit });

// TODO: ugly workaround - not sure why this is necessary.
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious if this is actually necessary? What happens if we don't have it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was planning to remove that code as we refactor more tests. But will remove as it does not break this test.

Removed legacy safety code from compile and deploy module
@ccowell ccowell closed this Dec 17, 2018
@davidmurdoch davidmurdoch deleted the issue-173 branch April 9, 2019 19:09
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

2 participants