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

Modular 02-client #6515

Closed
wants to merge 1 commit into from
Closed

Modular 02-client #6515

wants to merge 1 commit into from

Conversation

AdityaSripal
Copy link
Member

Description

  • Adds interface methods to ClientState and MsgCreateClient to keep 02-client modular
  • Leaves local-host client as a "special case" with its own handling. This makes sense because local-host handling is so different from everything else. Better to leave it as a special case and make all "foreign" clients conform to a cleaner interface

TODO:

  • fix 07-tendermint/types tests

closes: #6064


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • 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

@AdityaSripal AdityaSripal added WIP T: Dev UX UX for SDK developers (i.e. how to call our code) labels Jun 26, 2020
@codecov
Copy link

codecov bot commented Jun 26, 2020

Codecov Report

Merging #6515 into master will decrease coverage by 1.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #6515      +/-   ##
==========================================
- Coverage   56.36%   55.27%   -1.09%     
==========================================
  Files         470      383      -87     
  Lines       28234    22793    -5441     
==========================================
- Hits        15913    12598    -3315     
+ Misses      11195     9235    -1960     
+ Partials     1126      960     -166     

@colin-axner
Copy link
Contributor

see comment specifically:

moving consensus state outside of client state results in:
change the CheckHeaderAndUpdateState to receive in the client store so the correct consensus state can be retrieved. This maybe have ramifications with trying to make 02-client modular, see issue#6064

it may be useful for clients to have access to their consensus state in order to update

@AdityaSripal
Copy link
Member Author

We still wouldn't need to pass in clientstore, just make caller query for latest consensus state and pass it in. Though I'm in favor of keeping lates consensus-state in client state

@alexanderbez alexanderbez removed the WIP label Jul 7, 2020
@fedekunze fedekunze marked this pull request as draft July 7, 2020 10:28
@colin-axner colin-axner added the S:blocked Status: Blocked label Jul 20, 2020
@colin-axner
Copy link
Contributor

We should remove GetClientType interface func with this pr, I don't see any reason for it's use? Should just use the underlying struct type if needed

@AdityaSripal
Copy link
Member Author

Closing in favor of #7028

@colin-axner colin-axner deleted the aditya/modular-client branch October 8, 2020 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S:blocked Status: Blocked T: Dev UX UX for SDK developers (i.e. how to call our code)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x/ibc Make 02-client more modular
3 participants