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

Problem (fix #283): ibc-transfer not directly tested with chain-maind #286

Merged
merged 1 commit into from
Dec 2, 2020
Merged

Problem (fix #283): ibc-transfer not directly tested with chain-maind #286

merged 1 commit into from
Dec 2, 2020

Conversation

yihuang
Copy link
Collaborator

@yihuang yihuang commented Dec 1, 2020 •
edited
Loading

Solution:

  • start relayer start together with chain binaries
  • send ibc-transfer transaction with chain binary

👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻

PR Checklist:

  • Have you read the CONTRIBUTING.md?
  • Does your PR follow the C4 patch requirements?
  • Have you rebased your work on top of the latest master?
  • Have you checked your code compiles? (make)
  • Have you included tests for any non-trivial functionality?
  • Have you checked your code passes the unit tests? (make test)
  • Have you checked your code formatting is correct? (go fmt)
  • Have you checked your basic code style is fine? (golangci-lint run)
  • If you added any dependencies, have you checked they do not contain any known vulnerabilities? (go list -json -m all | nancy sleuth)
  • If your changes affect the client infrastructure, have you run the integration test?
  • If your changes affect public APIs, does your PR follow the C4 evolution of public contracts?
  • If your code changes public APIs, have you incremented the crate version numbers and documented your changes in the CHANGELOG.md?
  • If you are contributing for the first time, please read the agreement in CONTRIBUTING.md now and add a comment to this pull request stating that your PR is in accordance with the Developer's Certificate of Origin.

Thank you for your code, it's appreciated! :)

@lgtm-com
Copy link

lgtm-com bot commented Dec 1, 2020

This pull request introduces 1 alert when merging 38f97f5 into f91257f - view on LGTM.com

new alerts:

  • 1 for Unused import

@codecov
Copy link

codecov bot commented Dec 1, 2020 •
edited
Loading

Codecov Report

Merging #286 (60a05b0) into master (f91257f) will decrease coverage by 0.24%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #286      +/-   ##
==========================================
- Coverage   16.66%   16.41%   -0.25%     
==========================================
  Files          45       45              
  Lines        5671     5671              
==========================================
- Hits          945      931      -14     
- Misses       4476     4477       +1     
- Partials      250      263      +13
Flag Coverage Δ
integration_tests 16.41% <ø> (-0.25%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
x/supply/types/query.pb.go 5.91% <0.00%> (-0.55%) ⬇️
x/supply/keeper/grpc_query.go 13.59% <0.00%> (ø)

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 f91257f...60a05b0. Read the comment docs.

tomtau
tomtau approved these changes Dec 1, 2020
View reviewed changes
Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

lgtm

nix/sources.json
"type": "tarball",
"url": "https://github.com/yihuang/relayer/archive/5e9fd91198e206fe85be6214da68f4250b557063.tar.gz",
"url": "https://github.com/yihuang/relayer/archive/bdb89886db510626bbd1a9d836fd530dea7d1409.tar.gz",
Copy link
Contributor

Choose a reason for hiding this comment

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

that's coin type mainly cosmos/relayer@master...yihuang:cryptocom ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's coin type mainly cosmos/relayer@master...yihuang:cryptocom ?

Both cosmos/relayer#325 and cosmos/relayer#333

pystarport/pystarport/cluster.py Outdated
to,
amount,
"-y",
"--absolute-timeouts", # FIXME avoid an ClientState decoding error
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it needs fixing in SDK module?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

probably, that needs further investigation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cosmos/cosmos-sdk#8059
submitted bug report

@yihuang yihuang force-pushed the test-raw-ibc-transfer branch 2 times, most recently from 62412b2 to 645ea1e Compare December 2, 2020 03:03
tomtau
tomtau reviewed Dec 2, 2020
View reviewed changes
pystarport/pystarport/cluster.py Outdated
to,
amount,
"-y",
"--absolute-timeouts", # FIXME https://github.com/cosmos/cosmos-sdk/issues/8059
Copy link
Contributor

Choose a reason for hiding this comment

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

ignore lint or put it above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@yihuang
Problem (fix crypto-org-chain#283): ibc-transfer not directly tested …
60a05b0 
…with chain-maind

Solution:
- start `relayer start` together with chain binaries
- send ibc-transfer transaction with chain binary
@tomtau tomtau merged commit 3adccd8 into crypto-org-chain:master Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomtau tomtau tomtau approved these changes

@calvinaco calvinaco Awaiting requested review from calvinaco

@devashishdxt devashishdxt Awaiting requested review from devashishdxt

@leejw51crypto leejw51crypto Awaiting requested review from leejw51crypto

@linfeng-crypto linfeng-crypto Awaiting requested review from linfeng-crypto

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yihuang @tomtau