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

Remove GetHeight from ClientMessage interface #594

Closed
2 of 3 tasks
colin-axner opened this issue Dec 6, 2021 · 4 comments
Closed
2 of 3 tasks

Remove GetHeight from ClientMessage interface #594

colin-axner opened this issue Dec 6, 2021 · 4 comments
Assignees
Labels
02-client core needs discussion Issues that need discussion before they can be worked on type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.

Comments

@colin-axner
Copy link
Contributor

colin-axner commented Dec 6, 2021

Summary

Following implementation of #284, #285, and #286, GetHeight will no longer be necessary. This removal is necessary for the Wasm Light Client

Remove GetHeight from the Header ClientMessage interface. Usage in 07-tendermint can use the casted header function (cast to 07-tendermint type and use the concrete function)

Should be done after no longer setting the consensus state in updates in 02-client


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@colin-axner colin-axner self-assigned this Dec 6, 2021
@colin-axner colin-axner changed the title Remove GetLatestHeight from client state interface Remove GetHeight from header interface Dec 7, 2021
@crodriguezvega crodriguezvega added this to the v4.0.0 milestone Dec 16, 2021
@crodriguezvega crodriguezvega added the type: refactor Architecture, code or CI improvements that may or may not tackle technical debt. label Jan 3, 2022
@colin-axner colin-axner removed their assignment Feb 7, 2022
faddat pushed a commit to notional-labs/ibc-go that referenced this issue Feb 23, 2022
@crodriguezvega crodriguezvega added the needs discussion Issues that need discussion before they can be worked on label Apr 4, 2022
@crodriguezvega
Copy link
Contributor

crodriguezvega commented Apr 4, 2022

Relayer operators might need consensusHeight (see comment).

@damiannolan damiannolan self-assigned this Apr 20, 2022
@damiannolan damiannolan changed the title Remove GetHeight from header interface Remove GetHeight from ClientMessage interface Apr 20, 2022
@damiannolan
Copy link
Member

Updated issue to reflect new interface naming 👍

@damiannolan
Copy link
Member

After the discussion in #1208 linked above, the proposed solution moving forward is as follows:

  • Remove the GetHeight interface method from ClientMessage.
  • Return a list of heights []exported.Height from the new UpdateState interface.
  • Maintain the consensus_height event emitted from 02-client client update handler, but mark as deprecated for future removal. For example, with tendermint lightclients this will simply be heights[0] following a successful update.
  • Add an additional consensus_heights event, containing a list of updated heights. This provides flexibility for emitting multiple consensus heights from batched header updates.

@colin-axner
Copy link
Contributor Author

closed! Great work @damiannolan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
02-client core needs discussion Issues that need discussion before they can be worked on type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.
Projects
Status: Done 🥳
Development

No branches or pull requests

4 participants