Implement ValidationContext and ExecutionContext for clients (ICS-2)
#241
Conversation
plafer
changed the title
ValidationContext and ExecutionContextValidationContext and ExecutionContext for clients (ICS-2)
.changelog/unreleased/breaking-changes/240-validation-execution-contexts.md
Outdated
Show resolved
Hide resolved
| client_state, | ||
| consensus_state, | ||
| } = client_state | ||
| .check_header_and_update_state(ctx, client_id.clone(), header.clone()) |
There was a problem hiding this comment.
I think we shouldn't be running the complete header verification here and should only decode the header to obtain new client and consensus states. https://github.com/anoma/namada/blob/main/shared/src/ledger/ibc/handler.rs#L1187
There was a problem hiding this comment.
Right. Should we do the check_header_and_update_state() split from #173 first then? Or perhaps merge this but open an issue, which we address before the next release?
There was a problem hiding this comment.
I am okay with both, but I would prefer the second option (i.e. merge this and open an issue).
| let _ = client_state | ||
| .check_header_and_update_state(ctx, client_id.clone(), header) | ||
| .map_err(|e| Error::header_verification_failure(e.to_string()))?; | ||
|
|
There was a problem hiding this comment.
IIUC, the validate() function is Anoma's VP equivalent right? If so, the logic is incomplete, for e.g. w're missing verifying the previously emitted event (emitted during execution). https://github.com/anoma/namada/blob/main/shared/src/ledger/ibc/vp/client.rs#L311
There was a problem hiding this comment.
I see Anoma's VP as a superset of validate(). For example, "verifying that the event was previously emitted" is specific to Anoma's architecture, and doesn't belong in ibc-rs. So Anoma's VP would first call validate() (IBC-specific checks), and then do their own custom checks (suck as making sure the event was properly emitted).
| })?; | ||
| let consensus_state = client_state.initialise(consensus_state)?; | ||
|
|
||
| ctx.store_client_type(ClientTypePath(client_id.clone()), client_type.clone())?; |
There was a problem hiding this comment.
Would be nice if we could extract blocks like these (e.g. write_create_client(), emit_create_client_event(), etc.) and reused them in the future to avoid code duplication.
…-2) (#241) * ValidationContext scaffolding * validate for create_client() * update_client validate() * update client * add `ExecutionContext` * use pub(crate) * execute scaffolding * upgrade_client validate * execute() for create_client * execute update_client * upgrade_client execute * changelog * fmt * changelog fix
Closes: #240
PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.