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

Preliminary support for local keys #239

Merged
merged 10 commits into from
Aug 25, 2020
Merged

Conversation

dtebbs
Copy link
Contributor

@dtebbs dtebbs commented Aug 12, 2020

  • use eth-private-key if present to locally sign the tx (as well as eth-address as before)
  • test commands/gen_eth_address.py generates eth-private-key and eth-address in the current dir
  • test_commands/fund_eth_address.py funds the address, (assumes an the address is unlocked, funds from web3.eth.accounts[0] unless told otherwise) assuming gnache / geth with hosted accounts.
  • test scripts (e.g. scripts/test_zeth_cli call setup_user in scripts/test_zeth_client_common.sh to extract a (funded) hosted address from the RPC host. Create two setup functions setup_user_local_key (userd for alice, bob, charlie in the test script) and setup_user_hosted_key (used for deployer in the test scripts)
  • Make deploy command also support eth-private-key
  • [LOW PRIORITY] Make get_balances in the test scripts (scripts/test_zeth_client_common.sh) use the correct addresses
  • Deployment of test token, and token approval using local private keys

@dtebbs dtebbs changed the title WIP: preliminary support for local keys WIP: preliminary support for local keys (depends on #237) Aug 12, 2020
@dtebbs dtebbs force-pushed the zeth-client-key-management branch 2 times, most recently from 472d6d5 to 888e46e Compare August 12, 2020 15:50
Copy link
Contributor

@AntoineRondelet AntoineRondelet left a comment

Choose a reason for hiding this comment

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

Here's a quick one even if still WIP

client/commands/constants.py Outdated Show resolved Hide resolved
client/test_commands/deploy_test_token.py Outdated Show resolved Hide resolved
client/test_commands/deploy_test_token.py Show resolved Hide resolved
client/commands/zeth_deploy.py Outdated Show resolved Hide resolved
client/commands/zeth_token_approve.py Outdated Show resolved Hide resolved
@dtebbs dtebbs changed the base branch from serialization to develop August 21, 2020 16:50
@dtebbs dtebbs changed the title WIP: preliminary support for local keys (depends on #237) Preliminary support for local keys (depends on #237) Aug 21, 2020
@dtebbs dtebbs changed the title Preliminary support for local keys (depends on #237) Preliminary support for local keys Aug 24, 2020
@AntoineRondelet
Copy link
Contributor

@dtebbs there's a box that isn't ticked yet in your PR description. Do I need to treat this PR as still WIP or did you forget to tick the box?

@dtebbs
Copy link
Contributor Author

dtebbs commented Aug 24, 2020

@dtebbs there's a box that isn't ticked yet in your PR description. Do I need to treat this PR as still WIP or did you forget to tick the box?

@AntoineRondelet Sorry - I forgot to update it. Good catch.
The functionality was added, so should be good to go. Thanks.

client/commands/utils.py Outdated Show resolved Hide resolved
help=f"Ethereum rpc end-point")
"--eth-network",
default=None,
help="Ethereum RPC endpoint, network or config file "
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be possible to stick to one type of values here? I mean: do we really need the 3 potential value types?
I find it confusing to see that a flag can take 3 type of values (a URL or a "name" or a file).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely get the point. I'm sure it can be improved with more time invested, but my thinking was that each of these forms is a potential use-case, and there should not really be any ambiguity when specifying this. I considered having multiple flags to cover different types, but that would involve quite a bit more complexity and logic to resolve some ambiguous cases like multiple flags being given, overriding each other and falling though to defaults, etc.

So this seemed like a simple solution that avoids ambiguity, but definitely happy to iterate if there is a better way to expose the same functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, I'll open a ticket to keep this in mind and polish this going forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened #249 to track this

client/commands/zeth_mix.py Outdated Show resolved Hide resolved
@dtebbs
Copy link
Contributor Author

dtebbs commented Aug 25, 2020

(pushed a small fix for the client syncing code, but no other changes planned now)

@AntoineRondelet AntoineRondelet merged commit a50d897 into develop Aug 25, 2020
AntoineRondelet added a commit that referenced this pull request Aug 25, 2020
Client changes to suppot geth-style nodes, such as autonity (depends on #239)
@AntoineRondelet AntoineRondelet deleted the zeth-client-key-management branch September 1, 2020 12:22
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