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

Add UpdateState to ClientState interface #1194

Closed
3 tasks
colin-axner opened this issue Mar 29, 2022 · 3 comments
Closed
3 tasks

Add UpdateState to ClientState interface #1194

colin-axner opened this issue Mar 29, 2022 · 3 comments
Assignees

Comments

@colin-axner
Copy link
Contributor

Summary

Should be done after #1193 and #1190 . Add UpdateState to the ClientState interface. If #1193 added the call to UpdateStateOnMisbehaviour before CheckHeaderAndUpdateState then the same change can be made


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@damiannolan
Copy link
Member

I think we previously discussed removing the return values. However, returned ClientState is used for event emission, similarly for misbehaviour event emission.

@colin-axner
Copy link
Contributor Author

Hmm. Not returning the ClientState makes the API clean and it is a bit more clear to developers they need to set the ClientState. Returning the ClientState does allow for one less store query/less gas used, but it does open the possibility to a developer returning a different client state than that set in state (although hopefully not problematic if the returned client state is used for events only)

@seantking
Copy link
Contributor

Hmm. Not returning the ClientState makes the API clean and it is a bit more clear to developers they need to set the ClientState. Returning the ClientState does allow for one less store query/less gas used, but it does open the possibility to a developer returning a different client state than that set in state (although hopefully not problematic if the returned client state is used for events only)

This is a good point. I'm actually more fond of APIs that return the updated values but as you point out this may not actually be the case.

I'm fine with leaving it out and doing an additional query to the store.

@damiannolan damiannolan self-assigned this Mar 30, 2022
@crodriguezvega crodriguezvega moved this from Todo to In review in ibc-go Mar 30, 2022
@crodriguezvega crodriguezvega added this to the 02-client refactor milestone Mar 30, 2022
Repository owner moved this from In review to Done in ibc-go Mar 31, 2022
@crodriguezvega crodriguezvega moved this from Todo to Done in ibc-go Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done 🥳
Development

No branches or pull requests

4 participants