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 Route method in favour of Verify(Non)Membership on client keeper #6019

Closed
damiannolan opened this issue Mar 18, 2024 · 2 comments · Fixed by #6760
Closed

Remove Route method in favour of Verify(Non)Membership on client keeper #6019

damiannolan opened this issue Mar 18, 2024 · 2 comments · Fixed by #6760
Assignees
Labels
02-client type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.
Milestone

Comments

@damiannolan
Copy link
Member

damiannolan commented Mar 18, 2024

food for thought.. I wonder if we should rather be exposing VerifyMembership and VerifyNonMembership as client keeper methods now since in other places in 03-connection/04-channel where we did clientState look ups to do things like status, latest height, timestamp at height etc, we're now using client keeper methods which abstract away the router lookups.

I feel like this might be more in line with 02-client's purpose as a router in general. If dependent modules need to do custom things there is a GetRouter method exposed in 02-client

Interested to hear what others think 🤔

Originally posted by @damiannolan in #6013 (comment)

@damiannolan damiannolan added needs discussion Issues that need discussion before they can be worked on 02-client labels Mar 18, 2024
@damiannolan damiannolan moved this to Todo 🏃 in ibc-go Mar 18, 2024
@damiannolan damiannolan moved this from Todo 🏃 to Backlog 🕐 in ibc-go Mar 18, 2024
@damiannolan damiannolan added this to the v9.0.0 milestone Mar 18, 2024
@crodriguezvega crodriguezvega added the type: refactor Architecture, code or CI improvements that may or may not tackle technical debt. label May 16, 2024
@colin-axner
Copy link
Contributor

Sounds like a good idea to me 👍

@crodriguezvega crodriguezvega moved this from Backlog 🕐 to Todo 🏃 in ibc-go Jun 23, 2024
@crodriguezvega
Copy link
Contributor

I believe this is what needs to be done, please correct me if I am wrong:

VerifyMembership(
  ctx sdk.Context,
  clientID string,
  height Height,
  delayTimePeriod uint64,
  delayBlockPeriod uint64,
  proof []byte,
  path Path,
  value []byte,
) error
VerifyNonMembership(
  ctx sdk.Context,
  clientID string,
  height Height,
  delayTimePeriod uint64,
  delayBlockPeriod uint64,
  proof []byte,
  path Path,
) error
  • Implement the above functions. Get the client module like this (this could be extracted to a helper unexported function, since it will be repeated in a few places), and then call the corresponding function.
  • Use the functions implemented above in the Verify... functions of 03-connection.

@crodriguezvega crodriguezvega removed the needs discussion Issues that need discussion before they can be worked on label Jun 23, 2024
@gjermundgaraba gjermundgaraba self-assigned this Jun 26, 2024
@gjermundgaraba gjermundgaraba moved this from Todo 🏃 to In progress 👷 in ibc-go Jun 26, 2024
@crodriguezvega crodriguezvega moved this from In progress 👷 to In review 👀 in ibc-go Jul 6, 2024
@github-project-automation github-project-automation bot moved this from In review 👀 to Done 🥳 in ibc-go Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
02-client type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants