Implemented 'keys add' and 'keys list' commmands #408

andynog · 2020-11-19T22:34:14Z

Closes: #363

Description

This PR includes the implementation of the keys add and keys list commands on the relayer. Changes:

Removed all the -k parameter from the tx commands. So not key_seed.json file needs to be specified for a tx raw command
Refactored keybase logic to simplify methods, it has now the chain config so most parameters are used from the config
Implemented a keys add command. Basically this adds the key_seed.json file on a folder $HOME/.rrly. The file will be saved with the key_name parameter specified in the config (e.g. testkey.json).
The key file is stored un-encrypted for now. No integration at this time with an OS keyring that would safely store the file. Once the keyring logic is properly implemented then this file can be properly stored in a secure way. For now, the purpose is just to simplify the logic for the tx raw commands.
Added instructions on relayer-cli README.md file
Also implemented a keys list command that can be used to list the key configured for a chain

For contributor use:

Updated the Unreleased section of CHANGELOG.md with the issue.
If applicable: Unit tests written, added test to CI.
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Updated relevant documentation (docs/) and code comments.
Re-reviewed Files changed in the Github PR explorer.

…w command (#337)

…in local branch after merging (#363)

- Added a new test keyring backend to support adding keys to file system (under home folder) - Refactored logic to add key to be part of the keystore and not the command - Switched the keybase on a chain to use the test keyring - Key seed file is saved in the test keystore default folder (/home/andy/.rrly)

…ed name parameter from keys add cmd (#363)

- Clean up remaining key_seed flag for tx cmds - Refactored keybase to include chain config - Refactoring keyring methods to use chain config - Logic to use configured key to sign tx

…was having before merge. (#363)

codecov-io · 2020-11-19T22:51:12Z

Codecov Report

Merging #408 (ac6e485) into master (b1b37f5) will increase coverage by 23.0%.
The diff coverage is 67.5%.

@@            Coverage Diff            @@
##           master    #408      +/-   ##
=========================================
+ Coverage    13.6%   36.7%   +23.0%     
=========================================
  Files          69     154      +85     
  Lines        3752   11089    +7337     
  Branches     1374    4124    +2750     
=========================================
+ Hits          513    4072    +3559     
- Misses       2618    6360    +3742     
- Partials      621     657      +36

Impacted Files	Coverage Δ
modules/src/events.rs	`0.0% <0.0%> (ø)`
modules/src/ics02_client/events.rs	`0.0% <ø> (ø)`
modules/src/ics02_client/msgs.rs	`0.0% <0.0%> (ø)`
modules/src/ics02_client/raw.rs	`0.0% <0.0%> (ø)`
modules/src/ics03_connection/error.rs	`13.6% <0.0%> (-19.7%)`	⬇️
modules/src/ics04_channel/error.rs	`75.0% <ø> (+50.0%)`	⬆️
modules/src/ics04_channel/msgs/chan_open_try.rs	`67.0% <ø> (ø)`
modules/src/ics04_channel/msgs/recv_packet.rs	`63.5% <ø> (ø)`
modules/src/ics04_channel/msgs/timeout.rs	`69.2% <ø> (ø)`
modules/src/ics04_channel/packet.rs	`70.4% <ø> (+70.4%)`	⬆️
... and 282 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f71413...d3ed4da. Read the comment docs.

romac

Looks good, aside from some minor remarks. Thanks :)

romac · 2020-11-20T13:37:49Z

relayer-cli/src/commands/query/channel.rs

@@ -121,7 +121,7 @@ mod tests {
    #[test]
    fn parse_channel_query_end_parameters() {
        let default_params = QueryChannelEndCmd {
-            chain_id: Some("ibc0".to_string().parse().unwrap()),


Why is this needed?

I didn't implement this as part of my issue, maybe something merged from master? Maybe @ancazamfir knows ?

romac · 2020-11-20T13:38:40Z

relayer-cli/tests/fixtures/two_chains.toml

@@ -3,12 +3,13 @@ timeout = '10s'
 strategy = 'naive'

 [[chains]]
-id = 'ibc0'
+id = 'ibc-0'


Is this change needed because the identifiers of the chains spawned by the Go relayer setup scripts have changed?

Yes, they changed

Hmm, i don't understand why this needs change. We are still on stargate-4. This change is coming in stargate-5.

do you want me to change this back to ibc0 ?

relayer/src/keyring/store.rs

romac · 2020-11-20T13:41:41Z

relayer/src/keyring/store.rs

-    MemoryKeyStore { store: BTreeMap<Address, KeyEntry> },
+    MemoryKeyStore {
+        store: BTreeMap<String, String>,
+        chain_config: ChainConfig,


Do we really need to full chain config here? Can we instead just store the data that's actually needed from it (eg. the key name)?

Actually I had it before with key name and chain id but it made calling the functions more complex and thought that this way was cleaner and also if we need more params in the config here in the future this would make it simpler to implement

But I couldn't find a good pattern here. Ideally this is used as a property of the chain (keybase) so if the config could be "passed down" into the keybase then this chain_config property would not be needed.

andynog · 2020-11-20T19:15:59Z

I noticed that with other PRs merged to master this PR got broken again, I will fix this one again so it can be merged

ancazamfir

Found some issues while testing. I can only submit transactions on one chain (ibc0 of stargate-4/ gaia setup with Go relayer). For the other one the account is not recognized. Not sure why, will try to debug.
Also not sure why the protos had changed compared with master.

ancazamfir · 2020-11-20T19:34:07Z

relayer-cli/tests/fixtures/two_chains.toml

@@ -3,12 +3,13 @@ timeout = '10s'
 strategy = 'naive'

 [[chains]]
-id = 'ibc0'
+id = 'ibc-0'


Hmm, i don't understand why this needs change. We are still on stargate-4. This change is coming in stargate-5.

andynog · 2020-11-20T20:08:07Z

Found some issues while testing. I can only submit transactions on one chain (ibc0 of stargate-4/ gaia setup with Go relayer). For the other one the account is not recognized. Not sure why, will try to debug.
Also not sure why the protos had changed compared with master.

I've changed to ibc-0 because I was testing against the latest Go relayer release that now is on https://github.com/cosmos/relayer. For some reason their scripts to start two chains changed the chain-id to ibc-0. But if we are not using that to test then we can revert back to ibc0.

Concerning the transactions, please make sure you add the right key for each chain. So you should have a key_seed.json for each chain, after you add them you should see something like this under your $HOME/.rrly folder

$tree $HOME/.rrly
$HOME/.rrly/
└── keys
    ├── ibc-0
    │   └── keyring-test
    │       └── testkey.json
    └── ibc-1
        └── keyring-test
            └── testkey.json

But each testkey.json should have different contents. You can verify by doing:

relayer -c relayer-cli/tests/fixtures/two_chains.toml keys list ibc-0
keys list result:  "chain: ibc-0 -> testkey (cosmos1hkza0857rpkfl4uvwptv87he5t7f9g5v5yel7r)"

relayer -c ./relayer-cli/tests/fixtures/two_chains.toml keys list ibc-1
keys list result:  "chain: ibc-1 -> testkey (cosmos1hud8teqlrzw6zstnz3vf2qns547enm26ul22fw)"

which should display different addresses. Then the transactions should be signed with the key for the chain (right now there's no way to use more than one key per chain). If you have the right setup and still not working then might be a bug in the logic.

Concerning the proto changes, as far as I remember I didn't change them, might be changes once I've merged the latest master into this branch.

…stion (#408)

ancazamfir · 2020-11-20T22:21:37Z

Concerning the transactions, please make sure you add the right key for each chain. So you should have a key_seed.json for each chain, after you add them you should see something like this under your $HOME/.rrly folder

yes, I did that. All accounts and keys seem fine and I doubled checked they are the same as the ones from the Go relayer seed files. Does this work for you? Having both chains accepting transactions?

ancazamfir · 2020-11-20T22:24:26Z

I've changed to ibc-0 because I was testing against the latest Go relayer release that now is on https://github.com/cosmos/relayer. For some reason their scripts to start two chains changed the chain-id to ibc-0. But if we are not using that to test then we can revert back to ibc0.

Yes but for the latest (stargate-5) we need a number of changes in ibc-rs and tendermint-rs (I made changes for this on anca/stargate-5 on both). With current we cannot do proper client, connection and channel handshakes. IIRC only ConnOpenInit works.

ancazamfir · 2020-11-21T00:09:47Z

Concerning the transactions, please make sure you add the right key for each chain. So you should have a key_seed.json for each chain, after you add them you should see something like this under your $HOME/.rrly folder

yes, I did that. All accounts and keys seem fine and I doubled checked they are the same as the ones from the Go relayer seed files. Does this work for you? Having both chains accepting transactions?

I figured what went wrong. From previous testing I had some sections [[chains.peers.light_clients]] in the config file and for ibc1 one of the client peers was configured with the port of ibc0. This caused the transaction to be sent to ibc0. I will still have to investigate why, as the rpc address was correctly configured and the light clients are not currently started with the CLIs.
The good news is that your changes work, I tested almost all CLIs. The problem is that master is now broken after the upgrade to tokio 0.3 (compiles but CLIs are broken) so we should fix that before we merge your PR.

ancazamfir

Looks great! All commands work without the key seed file option, which is great! Thanks a lot Andy, let's merge this today.

* Added logic to generate GRPC client from cosmos.auth proto (#337) * Adding logic to use GRPC client (#337) * Grpc client connection retrieves account sequence (#337) * Removed the account sequence flag from the tx raw commands (#337) * Removed instructions to query and specify account sequence from tx raw command (#337) * Logic to fetch GRPC endpoint address from config (#337) * Fixing tests (#361) * Logic to use the address from the key seed (#337) * Added boilerplate code for a keys add command to the relayer (#363) * Removing key flag from tx cmds * Adding logic to get key specified in the config * Logic to get the key specified in the config (#363) * Removed the -k flag from the tx raw commands (#363) * More logic to add key command (#363) * key add command for memory store working (#363) * Added logic to persist key seed in 'home' folder (#363) * Changes implemented (#363): - Added a new test keyring backend to support adding keys to file system (under home folder) - Refactored logic to add key to be part of the keystore and not the command - Switched the keybase on a chain to use the test keyring - Key seed file is saved in the test keystore default folder (/home/andy/.rrly) * Logic to use the key_name parameter from the config to add key. Removed name parameter from keys add cmd (#363) * Changed the logic to get the key from the test keyring file store (#363) * Implemented changes: (#363) - Clean up remaining key_seed flag for tx cmds - Refactored keybase to include chain config - Refactoring keyring methods to use chain config - Logic to use configured key to sign tx * Updated the README instructions (#363) * Disable the 'keys restore' command for now (#363) * Added 'keys list' command to show key added on a chain (#363) * Added entry for issue #363 (PR #408) * Refactored the bound variables to use the full name per comment suggestion (#408) * Move key retrieval, memo and timeout height inside send_tx * Add the client creation, connection and channel handshake * remove sleeps * More error handling, cleanup * Macro for channel CLIs * Macro for connection CLIs * Where src/dst make no sense rename to a/b, also fix a few bugs after last commits * cleanup * cargo fmt * Use Romain's skip-verif until backwards verification is done * fix CLI bugs Co-authored-by: Andy Nogueira <me@andynogueira.dev>

* Added logic to generate GRPC client from cosmos.auth proto (#337) * Adding logic to use GRPC client (#337) * Grpc client connection retrieves account sequence (#337) * Removed the account sequence flag from the tx raw commands (#337) * Removed instructions to query and specify account sequence from tx raw command (#337) * Logic to fetch GRPC endpoint address from config (#337) * Fixing tests (informalsystems#361) * Logic to use the address from the key seed (#337) * Added boilerplate code for a keys add command to the relayer (informalsystems#363) * Removing key flag from tx cmds * Adding logic to get key specified in the config * Logic to get the key specified in the config (informalsystems#363) * Removed the -k flag from the tx raw commands (informalsystems#363) * More logic to add key command (informalsystems#363) * key add command for memory store working (informalsystems#363) * Added logic to persist key seed in 'home' folder (informalsystems#363) * Changes implemented (informalsystems#363): - Added a new test keyring backend to support adding keys to file system (under home folder) - Refactored logic to add key to be part of the keystore and not the command - Switched the keybase on a chain to use the test keyring - Key seed file is saved in the test keystore default folder (/home/andy/.rrly) * Logic to use the key_name parameter from the config to add key. Removed name parameter from keys add cmd (informalsystems#363) * Changed the logic to get the key from the test keyring file store (informalsystems#363) * Implemented changes: (informalsystems#363) - Clean up remaining key_seed flag for tx cmds - Refactored keybase to include chain config - Refactoring keyring methods to use chain config - Logic to use configured key to sign tx * Updated the README instructions (informalsystems#363) * Disable the 'keys restore' command for now (informalsystems#363) * Added 'keys list' command to show key added on a chain (informalsystems#363) * Added entry for issue informalsystems#363 (PR informalsystems#408) * Refactored the bound variables to use the full name per comment suggestion (informalsystems#408) * Move key retrieval, memo and timeout height inside send_tx Co-authored-by: Anca Zamfir <zamfiranca@gmail.com>

* Added logic to generate GRPC client from cosmos.auth proto (#337) * Adding logic to use GRPC client (#337) * Grpc client connection retrieves account sequence (#337) * Removed the account sequence flag from the tx raw commands (#337) * Removed instructions to query and specify account sequence from tx raw command (#337) * Logic to fetch GRPC endpoint address from config (#337) * Fixing tests (informalsystems#361) * Logic to use the address from the key seed (#337) * Added boilerplate code for a keys add command to the relayer (informalsystems#363) * Removing key flag from tx cmds * Adding logic to get key specified in the config * Logic to get the key specified in the config (informalsystems#363) * Removed the -k flag from the tx raw commands (informalsystems#363) * More logic to add key command (informalsystems#363) * key add command for memory store working (informalsystems#363) * Added logic to persist key seed in 'home' folder (informalsystems#363) * Changes implemented (informalsystems#363): - Added a new test keyring backend to support adding keys to file system (under home folder) - Refactored logic to add key to be part of the keystore and not the command - Switched the keybase on a chain to use the test keyring - Key seed file is saved in the test keystore default folder (/home/andy/.rrly) * Logic to use the key_name parameter from the config to add key. Removed name parameter from keys add cmd (informalsystems#363) * Changed the logic to get the key from the test keyring file store (informalsystems#363) * Implemented changes: (informalsystems#363) - Clean up remaining key_seed flag for tx cmds - Refactored keybase to include chain config - Refactoring keyring methods to use chain config - Logic to use configured key to sign tx * Updated the README instructions (informalsystems#363) * Disable the 'keys restore' command for now (informalsystems#363) * Added 'keys list' command to show key added on a chain (informalsystems#363) * Added entry for issue informalsystems#363 (PR informalsystems#408) * Refactored the bound variables to use the full name per comment suggestion (informalsystems#408) * Move key retrieval, memo and timeout height inside send_tx * Add the client creation, connection and channel handshake * remove sleeps * More error handling, cleanup * Macro for channel CLIs * Macro for connection CLIs * Where src/dst make no sense rename to a/b, also fix a few bugs after last commits * cleanup * cargo fmt * Use Romain's skip-verif until backwards verification is done * fix CLI bugs Co-authored-by: Andy Nogueira <me@andynogueira.dev>

andynog added 26 commits November 3, 2020 14:37

Added logic to generate GRPC client from cosmos.auth proto (#337)

31bd354

Adding logic to use GRPC client (#337)

352a6d9

Grpc client connection retrieves account sequence (#337)

388c380

Removed the account sequence flag from the tx raw commands (#337)

57de7bc

Removed instructions to query and specify account sequence from tx ra…

3b54eb6

…w command (#337)

Logic to fetch GRPC endpoint address from config (#337)

49a787c

Fixing tests (#361)

16776a6

Logic to use the address from the key seed (#337)

15b46a1

Added boilerplate code for a keys add command to the relayer (#363)

0bf8230

Merge branch 'andy/acct-sequence' into andy/add-key-cmd

8ed590a

Removing key flag from tx cmds

f1b2e50

Adding logic to get key specified in the config

833a200

Logic to get the key specified in the config (#363)

6a38918

Removed the -k flag from the tx raw commands (#363)

164a130

More logic to add key command (#363)

3c89481

key add command for memory store working (#363)

1bc1046

Merging master into andy/add-key-cmd. Will re-implement changes lost …

f356a4d

…in local branch after merging (#363)

Added logic to persist key seed in 'home' folder (#363)

e60fa7c

Logic to use the key_name parameter from the config to add key. Remov…

03e9880

…ed name parameter from keys add cmd (#363)

Changed the logic to get the key from the test keyring file store (#363)

3eb5fd6

Implemented changes: (#363)

c695734

- Clean up remaining key_seed flag for tx cmds - Refactored keybase to include chain config - Refactoring keyring methods to use chain config - Logic to use configured key to sign tx

Merging master and fixing merging conflicts. Merging fixed tx issues …

6240317

…was having before merge. (#363)

Updated the README instructions (#363)

4fcec4a

Disable the 'keys restore' command for now (#363)

ce1aa53

Added 'keys list' command to show key added on a chain (#363)

cd1fd8b

andynog added I: CLI Internal: related to the relayer's CLI I: logic Internal: related to the relaying logic labels Nov 19, 2020

andynog requested review from romac and ancazamfir November 19, 2020 22:34

Added entry for issue #363 (PR #408)

15eb796

romac requested changes Nov 20, 2020

View reviewed changes

ancazamfir reviewed Nov 20, 2020

View reviewed changes

Refactored the bound variables to use the full name per comment sugge…

f363896

…stion (#408)

ancazamfir added 2 commits November 23, 2020 21:32

Merge branch 'master' into andy/add-key-cmd

ac6e485

Move key retrieval, memo and timeout height inside send_tx

d3ed4da

ancazamfir approved these changes Nov 24, 2020

View reviewed changes

romac approved these changes Nov 24, 2020

View reviewed changes

romac merged commit a3303c3 into master Nov 24, 2020

romac deleted the andy/add-key-cmd branch November 24, 2020 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented 'keys add' and 'keys list' commmands #408

Implemented 'keys add' and 'keys list' commmands #408

andynog commented Nov 19, 2020 •

edited

Loading

codecov-io commented Nov 19, 2020 •

edited

Loading

romac left a comment

romac Nov 20, 2020

andynog Nov 20, 2020

romac Nov 20, 2020

andynog Nov 20, 2020

ancazamfir Nov 20, 2020

andynog Nov 20, 2020

romac Nov 20, 2020

andynog Nov 20, 2020

andynog Nov 20, 2020

andynog commented Nov 20, 2020

ancazamfir left a comment

ancazamfir Nov 20, 2020

andynog commented Nov 20, 2020 •

edited

Loading

ancazamfir commented Nov 20, 2020

ancazamfir commented Nov 20, 2020

ancazamfir commented Nov 21, 2020

ancazamfir left a comment

Implemented 'keys add' and 'keys list' commmands #408

Implemented 'keys add' and 'keys list' commmands #408

Conversation

andynog commented Nov 19, 2020 • edited Loading

Description

codecov-io commented Nov 19, 2020 • edited Loading

Codecov Report

romac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andynog commented Nov 20, 2020

ancazamfir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andynog commented Nov 20, 2020 • edited Loading

ancazamfir commented Nov 20, 2020

ancazamfir commented Nov 20, 2020

ancazamfir commented Nov 21, 2020

ancazamfir left a comment

Choose a reason for hiding this comment

andynog commented Nov 19, 2020 •

edited

Loading

codecov-io commented Nov 19, 2020 •

edited

Loading

andynog commented Nov 20, 2020 •

edited

Loading