Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Update chain id arguments handling #129

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Update chain id arguments handling #129

wants to merge 9 commits into from

Conversation

r8d8
Copy link
Contributor

@r8d8 r8d8 commented Aug 27, 2018

Add new chain id/name to CLI (ETH related)
Related to #128

@ghost ghost assigned r8d8 Aug 27, 2018
@ghost ghost added the review label Aug 27, 2018
@r8d8 r8d8 closed this Oct 29, 2018
@r8d8 r8d8 deleted the issue/tx_sign branch October 29, 2018 13:44
@r8d8 r8d8 restored the issue/tx_sign branch October 29, 2018 13:49
@r8d8 r8d8 reopened this Oct 29, 2018
@r8d8 r8d8 requested review from whilei and tzdybal October 29, 2018 14:10
Copy link

@tzdybal tzdybal left a comment

Choose a reason for hiding this comment

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

LGTM

@splix splix self-requested a review October 29, 2018 19:06
README.md Outdated
@@ -44,7 +43,7 @@ FLAGS:

OPTIONS:
-p, --base-path <base-path> Set path for chain storage
-c, --chain <chain> Sets a chain name [default: mainnet]
-c, --chain <chain> Sets a chain name [default: etc]
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because ETH and ETC can't share same name

Copy link
Contributor

Choose a reason for hiding this comment

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

but ETH is ETH, not mainnet, it's different name. I mean it's ok to have etc as a synonym to mainnet, but it shouldn't eliminate mainnet, because it's what expected to use for mainnet

Copy link
Contributor Author

@r8d8 r8d8 Oct 31, 2018

Choose a reason for hiding this comment

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

ok, I see. Will make then prefix for specific blockchain (etc-..|eth-...) and leave as is for distinctive name.

cli.bats Outdated Show resolved Hide resolved
cli.bats Outdated
@test "succeeds: --chain=mainnet new --security=high --name='Test account' --description='Some description'" {
run $EMERALD_CLI --chain=mainnet \
@test "succeeds: --chain=etc-main new --security=high --name='Test account' --description='Some description'" {
run $EMERALD_CLI --chain=etc-main \
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

@r8d8 r8d8 Oct 31, 2018

Choose a reason for hiding this comment

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

Because ETH and ETC can't share same name

Copy link
Contributor

Choose a reason for hiding this comment

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

see above

| 31 | Rootstock testnet | rootstock-test |
| 42 | Kovan | kovan |
| 61 | Ethereum Classic mainnet | etc-main |
| 62 | Ethereum Classic testnet | etc-test |
Copy link
Contributor

Choose a reason for hiding this comment

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

geth uses morden, why vault has own testnet instead?

Copy link
Contributor Author

@r8d8 r8d8 Oct 31, 2018

Choose a reason for hiding this comment

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

ETH morden chain id is 2, ETC morden - 62

Copy link
Contributor

Choose a reason for hiding this comment

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

should (or is?) CLI argument etc be synonym for etc-main?

like eth is implicit for eth-main?

Copy link
Contributor

Choose a reason for hiding this comment

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

and let's use mainet/testnet instead of main/test, to avoid confusion. I mean etc-mainnet (as a synonym to simple mainnet), not etc-main which will be a totally new name

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

Successfully merging this pull request may close these issues.

4 participants