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

[ICS07] Investigate updating the store in ClientState::initialize() instead of create_client::execute() #614

Closed
Farhad-Shabani opened this issue Apr 12, 2023 · 1 comment · Fixed by #683
Labels
O: exploratory Objective: aims to investigate new ideas

Comments

@Farhad-Shabani
Copy link
Member

Farhad-Shabani commented Apr 12, 2023

Summary

Looking at the spec, we are missing the following check & setting upon initializing a Tendermint client state:

  • assert(clientState.trustLevel >= 1/3 && clientState.trustLevel <= 1)
  • provableStore.set("clients/{identifier}/clientState", clientState)
@Farhad-Shabani Farhad-Shabani added the A: bug Admin: something isn't working label Apr 12, 2023
@Farhad-Shabani Farhad-Shabani added the S: specs Scope: related to IBC protocol specifications label Apr 12, 2023
@Farhad-Shabani Farhad-Shabani changed the title [ICS07] Missing some client initializing checks according to the spec [ICS07] Missing some client initializing steps according to the spec Apr 12, 2023
@Farhad-Shabani Farhad-Shabani moved this to 📥 To Do in ibc-rs Apr 12, 2023
@plafer
Copy link
Contributor

plafer commented Apr 13, 2023

We do them, but in different but equivalent places:

assert(clientState.trustLevel >= 1/3 && clientState.trustLevel <= 1)

Here:

let _ = TendermintTrustThresholdFraction::new(
trust_level.numerator(),
trust_level.denominator(),
)

provableStore.set("clients/{identifier}/clientState", clientState)

Here:

ctx.store_client_state(ClientStatePath::new(&client_id), client_state.clone())?;

2 points:

  1. the spec is very often out of date. Looking at ibc-go instead, see that it does not do (1) there either, but does (2).
  2. Here, as in ClientState::{update_state, update_state_on_misbehaviour}(), they give the "mutating the store" responsibility to the light client instead of the core handler. I suspect that this comes as a realization that not all clients are updated the same (as we currently assume in ibc-rs). We should investigate whether that makes sense for us as well. Related: Investigate removing ValidationContext argument from light client methods #615

@plafer plafer added O: exploratory Objective: aims to investigate new ideas and removed A: bug Admin: something isn't working S: specs Scope: related to IBC protocol specifications labels Apr 13, 2023
@plafer plafer changed the title [ICS07] Missing some client initializing steps according to the spec [ICS07] Investigate updating the store in ClientState::initialize() instead of create_client::execute Apr 13, 2023
@plafer plafer changed the title [ICS07] Investigate updating the store in ClientState::initialize() instead of create_client::execute [ICS07] Investigate updating the store in ClientState::initialize() instead of create_client::execute() Apr 13, 2023
@github-project-automation github-project-automation bot moved this from 📥 To Do to ✅ Done in ibc-rs Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O: exploratory Objective: aims to investigate new ideas
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants