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

Relayer CLIs for client messages #251

Merged
merged 14 commits into from
Sep 25, 2020
Merged

Relayer CLIs for client messages #251

merged 14 commits into from
Sep 25, 2020

Conversation

ancazamfir
Copy link
Collaborator

Closes: #207

Description

Implements the relayer CLI to create a client.

Notes

Currently light client fails to get the header from the source chain (#90)
Also chain.send() is not fully implemented (#47)

For contributor use:

  • Unit tests written
  • Added test to CI if applicable
  • Updated CHANGELOG_PENDING.md
  • 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

romac
romac reviewed Sep 24, 2020
View reviewed changes
Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Great stuff! Some minor nitpicks inline, feel free to ignore/postpone. Not approving yet since it's still marked WIP.

modules/src/ics02_client/client_def.rs Outdated Show resolved Hide resolved
modules/src/ics02_client/handler/create_client.rs Outdated Show resolved Hide resolved
modules/src/ics02_client/handler/create_client.rs Outdated Show resolved Hide resolved
modules/src/ics02_client/handler/create_client.rs Outdated Show resolved Hide resolved
modules/src/ics02_client/handler/create_client.rs Outdated Show resolved Hide resolved
modules/src/ics02_client/msgs.rs Outdated Show resolved Hide resolved
relayer-cli/src/commands/tx.rs Show resolved Hide resolved
relayer-cli/src/commands/tx.rs Show resolved Hide resolved
relayer-cli/src/commands/tx/client.rs Outdated Show resolved Hide resolved
relayer/src/chain.rs Outdated Show resolved Hide resolved
@ancazamfir ancazamfir changed the title [WIP] Relayer CLIs for client messages Relayer CLIs for client messages Sep 24, 2020
adizere
adizere approved these changes Sep 24, 2020
View reviewed changes
Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

Wohoo awesome! I mostly had minor suggestions.

modules/src/ics02_client/error.rs Outdated Show resolved Hide resolved
modules/src/ics02_client/handler/create_client.rs Outdated Show resolved Hide resolved
modules/src/ics02_client/msgs.rs Outdated Show resolved Hide resolved
modules/src/ics02_client/msgs.rs Outdated Show resolved Hide resolved
modules/src/ics02_client/msgs.rs Outdated Show resolved Hide resolved
modules/src/ics02_client/msgs.rs Outdated Show resolved Hide resolved
modules/src/ics07_tendermint/error.rs Outdated Show resolved Hide resolved
modules/src/ics07_tendermint/error.rs Outdated Show resolved Hide resolved
modules/src/ics07_tendermint/consensus_state.rs Outdated Show resolved Hide resolved
@ancazamfir
Copy link
Collaborator Author

Thanks for the reviews @romac and @adizere! I tried to address all of them, please take another look if you have time.

@ancazamfir ancazamfir self-assigned this Sep 25, 2020
adizere
adizere approved these changes Sep 25, 2020
View reviewed changes
Copy link
Member

@adizere adizere 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, thanks Anca!

relayer/src/tx/client.rs Outdated
use crate::error::{Error, Kind};

#[derive(Clone, Debug)]
pub struct CreateClientStateOptions {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Was wondering why wasn't this called "CreateClientOptions". (The name "ClientState" appears in my mind as a contrast to "Client Consensus State".)

@codecov-commenter
Copy link

Codecov Report

Merging #251 into master will increase coverage by 21.6%.
The diff coverage is 54.1%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #251      +/-   ##
=========================================
+ Coverage    13.6%   35.2%   +21.6%     
=========================================
  Files          69     107      +38     
  Lines        3752    7284    +3532     
  Branches     1374    2688    +1314     
=========================================
+ Hits          513    2571    +2058     
- Misses       2618    4322    +1704     
+ Partials      621     391     -230
Impacted Files Coverage Δ
modules/src/events.rs 0.0% <ø> (ø)
modules/src/ics02_client/events.rs 0.0% <ø> (ø)
modules/src/ics02_client/raw.rs 0.0% <0.0%> (ø)
modules/src/ics03_connection/error.rs 16.6% <0.0%> (-16.7%) ⬇️
modules/src/ics04_channel/error.rs 75.0% <ø> (+50.0%) ⬆️
modules/src/ics04_channel/packet.rs 0.0% <0.0%> (ø)
modules/src/ics07_tendermint/client_def.rs 0.0% <0.0%> (ø)
modules/src/ics07_tendermint/error.rs 75.0% <ø> (+75.0%) ⬆️
modules/src/mock_client/header.rs 0.0% <0.0%> (ø)
modules/src/mock_client/state.rs 0.0% <0.0%> (ø)
... and 190 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 9357d66...31afb49. Read the comment docs.

@ancazamfir ancazamfir merged commit 2979c5a into master Sep 25, 2020
@ancazamfir ancazamfir deleted the anca/client_tx_clis branch September 25, 2020 17:03
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
@ancazamfir
Relayer CLIs for client messages (informalsystems#251)
f1e5103 
* relayer create client cli

* Add conversion between MsgCreateClient and MsgCreateAnyClient

* added forgotten file

* added forgotten file

* Move functionality in create_clien() to be used by the relayer

* Remove ics07 messages

* Cleanup

* Move create_clien() in the relayer

* Review comments

* Review comments

* Add new get_dummy_.. helper function

* Remove backtrace from the output

* Review comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romac romac romac left review comments

@adizere adizere adizere approved these changes

Assignees

@ancazamfir ancazamfir

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Relayer CLIs for client create messages
4 participants
@ancazamfir @codecov-commenter @romac @adizere