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

Fixes to make hermes work with non-gaiad custom chains (e.g. starport) #753

Merged
merged 3 commits into from
Mar 22, 2021

Conversation

andynog
Copy link
Contributor

@andynog andynog commented Mar 20, 2021

Closes:

Description

This PR implements fixes to allow hermes relay packets between two non-gaiad based chains.

These changes were tested against two starport v0.15.0 (supports cosmos-sdk v0.42.1). These starport chains have custom account prefix addresses (not cosmos) such as chainb1pnv0elanhs8qcq2sst0jssak8magjpj3ggq45x

This PR also fixes an issue where the account number is not 0. For example the starport chains have multiple accounts (e.g. validator and bob). So using bob account key in the relayer would make the transactions fail because that account number is 1 in the key store. After the fix, it works now because the account number is dynamically fetched from the chain using grpc (same logic that gets the account sequence to submit transaction).


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.

@andynog andynog added A: bug Admin: something isn't working I: CLI Internal: related to the relayer's CLI I: logic Internal: related to the relaying logic labels Mar 20, 2021
@andynog andynog added this to the 03.2021 milestone Mar 20, 2021
@andynog andynog requested review from romac, adizere and ancazamfir March 20, 2021 14:20
@codecov-io
Copy link

codecov-io commented Mar 20, 2021

Codecov Report

Merging #753 (a25d544) into master (b1b37f5) will increase coverage by 30.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #753      +/-   ##
=========================================
+ Coverage    13.6%   43.8%   +30.1%     
=========================================
  Files          69     159      +90     
  Lines        3752   10427    +6675     
  Branches     1374       0    -1374     
=========================================
+ Hits          513    4572    +4059     
- Misses       2618    5855    +3237     
+ Partials      621       0     -621     
Impacted Files Coverage Δ
...application/ics20_fungible_token_transfer/error.rs 0.0% <ø> (ø)
...ion/ics20_fungible_token_transfer/msgs/transfer.rs 0.0% <ø> (ø)
..._transfer/relay_application_logic/send_transfer.rs 0.0% <ø> (ø)
modules/src/events.rs 0.0% <ø> (ø)
modules/src/handler.rs 100.0% <ø> (ø)
modules/src/ics02_client/client_consensus.rs 55.8% <ø> (ø)
modules/src/ics02_client/client_def.rs 33.0% <ø> (ø)
modules/src/ics02_client/client_state.rs 65.1% <ø> (ø)
modules/src/ics02_client/client_type.rs 79.1% <ø> (+31.5%) ⬆️
modules/src/ics02_client/context.rs 100.0% <ø> (ø)
... and 255 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 9e68484...a25d544. Read the comment docs.

Copy link
Collaborator

@ancazamfir ancazamfir 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 Andy!

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.

Looks good, I just have one question but it does not block from merging.

@@ -181,7 +183,7 @@ impl CosmosSdkChain {
// Gas Fee
let coin = Coin {
denom: "stake".to_string(),
amount: "1000".to_string(),
amount: "10000".to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

Is this supposed to stay hardcoded forever or do we eventually want to make this configurable/dynamic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@romac I believe this should be dynamic, we can have a default value but should allow the value to be passed as a parameter, I had opened a separate issue for that #754

@romac romac merged commit 0dd0a03 into master Mar 22, 2021
@adizere adizere deleted the andy/starport branch March 23, 2021 14:53
@andynog andynog mentioned this pull request Mar 24, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: bug Admin: something isn't working I: CLI Internal: related to the relayer's CLI I: logic Internal: related to the relaying logic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants