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.

Sign up for GitHub

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

Jump to bottom

Add dev testnet. #141

Merged
merged 3 commits into from
Nov 1, 2022
Merged

Add dev testnet. #141

merged 3 commits into from
Nov 1, 2022

Conversation

dvc94ch
Copy link
Contributor

@dvc94ch dvc94ch commented Oct 3, 2022

Makes it easier to test stuff by adding support for running a local testnet.

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Oct 5, 2022

ping @GeekArthur. sorry never written any go before. did I do something incredibly stupid? I think this is a really useful patch for people building apps that use the rosetta api.

xiaying-peng
xiaying-peng reviewed Oct 5, 2022
View reviewed changes
configuration/configuration.go
Network: ethereum.DevNetwork,
}
config.GenesisBlockIdentifier = nil
config.Params = params.AllCliqueProtocolChanges
Copy link
Contributor

Choose a reason for hiding this comment

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

do we this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks to me like the config for the dev testnet: https://github.com/ethereum/go-ethereum/blob/master/params/config.go#L280

it has the right chain id 1337, all the protocol transitions are set to 0 and uses clique consensus.

@xiaying-peng
Copy link
Contributor

How do you connect Rosetta to your local testnet? Do you use run-testnet-remote?

@GeekArthur
Copy link

@dvc94ch I think it makes sense to me to enable geth developer mode for dApp development or smart contract development. The majority of current rosetta usage is around blockchain integration, I believe that's why we don't have dev mode as we need to leverage the large amount of data from pre-existing public network for validating the implementation of rosetta API

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Oct 6, 2022 •
edited
Loading

How do you connect Rosetta to your local testnet? Do you use run-testnet-remote?

I repurposed the TESTNET configuration which was previously an alias for ROPSTEN. Since it's not clear to me why ropsten is the testnet. If something were the testnet I'd think it would be a local testnet. Maybe there is something special about ropsten that makes it stand out as the default testnet.

To answer your question, you can test it using the command sequence below:

make build-local
make run-testnet-online
geth attach http://127.0.0.1:8545
> eth.sendTransaction({from: eth.coinbase, to: "0x0000011111222223333344444555556666677777", value: web3.toWei(50, "ether")})

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Oct 17, 2022

not a very exciting feature I guess. any estimate when you'll find time to review it?

GeekArthur
GeekArthur previously approved these changes Oct 17, 2022
View reviewed changes
@GeekArthur
Copy link

The PR LGTM, @xiaying-peng you can review again to see if it addresses your previous comments

xiaying-peng
xiaying-peng previously approved these changes Oct 17, 2022
View reviewed changes
Copy link
Contributor

@xiaying-peng xiaying-peng left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for contributing!

@dvc94ch dvc94ch dismissed stale reviews from xiaying-peng and GeekArthur via b193a1b October 17, 2022 18:59
@dvc94ch
Copy link
Contributor Author

dvc94ch commented Oct 22, 2022

If someone could reapprove so ci can run, that would be great

xiaying-peng
xiaying-peng reviewed Oct 22, 2022
View reviewed changes
configuration/configuration_test.go
@@ -148,14 +148,14 @@ func TestLoadConfiguration(t *testing.T) {
cfg: &Configuration{
Mode: Online,
Network: &types.NetworkIdentifier{
Network: ethereum.RopstenNetwork,
Network: ethereum.DevNetwork,
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add an additional test for devnet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, depends if NETWORK=TESTNET means devnet or ropsten. there still is a test for ropsten here https://github.com/coinbase/rosetta-ethereum/pull/141/files/1032355c75d5317c6324ee9fdea22fad474a62fb#diff-0aadb475e41b205384efdcf4231633cdd5097bb5b936000d7ce06989c0e41741L92

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah maybe you're asking how to enable the dev net? The only way currently is by setting NETWORK=TESTNET. we could also allow NETWORK=DEV or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xiaying-peng if you tell me what changes you'd like I'll make them. I'm not quite clear on what you'd like. We can leave TESTNET to mean ROPSTEN and add a DEV network, if that is what you want.

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Nov 1, 2022

can we merge this?

@xiaying-peng xiaying-peng self-requested a review November 1, 2022 20:50
xiaying-peng
xiaying-peng approved these changes Nov 1, 2022
View reviewed changes
Copy link
Contributor

@xiaying-peng xiaying-peng left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@xiaying-peng xiaying-peng merged commit 64f2f07 into coinbase:master Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaying-peng xiaying-peng xiaying-peng approved these changes

@GeekArthur GeekArthur GeekArthur left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dvc94ch @xiaying-peng @GeekArthur