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

x/ibc: Implement Timeout Timestamp #6022

Merged
merged 15 commits into from
Apr 22, 2020
Merged

x/ibc: Implement Timeout Timestamp #6022

merged 15 commits into from
Apr 22, 2020

Conversation

colin-axner
Copy link
Contributor

@colin-axner colin-axner commented Apr 17, 2020

Closes: #5836

Description

The timestamp represents the nanoseconds elapsed from initial unix time (Jan 1, 1970).


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@lgtm-com
Copy link

lgtm-com bot commented Apr 17, 2020

This pull request introduces 1 alert when merging e2e4878 into fe163e8 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

x/ibc/04-channel/keeper/packet.go Outdated Show resolved Hide resolved
x/ibc/04-channel/keeper/packet.go Outdated Show resolved Hide resolved
@colin-axner
Copy link
Contributor Author

colin-axner commented Apr 20, 2020

I need to add a default timeout timestamp in addition to the default timeout height in 20-transfers/keeper/keeper.go. Any suggestions?

Edit: Because of the variance in block time per chain, I think I'll just set it to 0, making the timeout be dependent upon the timeout height

@colin-axner
Copy link
Contributor Author

Also, should timestamp be in Unix nanoseconds or seconds? I initially started using nanoseconds, but I just noticed halt time on the SDK is in Unix seconds. Don't think there are strong arguments for one or the other, but might be best to be consistent despite the separation between ibc and sdk app

@colin-axner colin-axner changed the title WIP x/ibc: Implement Timeout Timestamp x/ibc: Implement Timeout Timestamp Apr 21, 2020
x/ibc/03-connection/keeper/keeper.go Outdated Show resolved Hide resolved
x/ibc/03-connection/keeper/keeper.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 21, 2020

Codecov Report

Merging #6022 into master will increase coverage by 0.01%.
The diff coverage is 61.29%.

@@            Coverage Diff             @@
##           master    #6022      +/-   ##
==========================================
+ Coverage   54.64%   54.66%   +0.01%     
==========================================
  Files         425      425              
  Lines       25822    25866      +44     
==========================================
+ Hits        14111    14139      +28     
- Misses      10737    10750      +13     
- Partials      974      977       +3     

@colin-axner colin-axner marked this pull request as ready for review April 21, 2020 23:44
@colin-axner
Copy link
Contributor Author

What is the change log work flow? Do I need to do update anything?

Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM, minor suggestions

x/ibc/02-client/exported/exported.go Outdated Show resolved Hide resolved
x/ibc/04-channel/keeper/packet.go Outdated Show resolved Hide resolved
x/ibc/04-channel/keeper/packet.go Outdated Show resolved Hide resolved
x/ibc/04-channel/types/packet.go Outdated Show resolved Hide resolved
x/ibc/07-tendermint/types/consensus_state.go Outdated Show resolved Hide resolved
@fedekunze fedekunze added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 22, 2020
@mergify mergify bot merged commit f6e9ee7 into cosmos:master Apr 22, 2020
@cwgoes
Copy link
Contributor

cwgoes commented Apr 23, 2020

This looks fine to me, thanks @colin-axner.

At some point, it might be nice to add an option to 20-transfer in the CLI to send using a timestamp timeout instead of a block height timeout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement timeout timestamp
4 participants