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

feat: detach client state verifier enabling users to implement custom verifier #1097

Merged
merged 24 commits into from
Feb 29, 2024

Conversation

seanchen1991
Copy link
Contributor

@seanchen1991 seanchen1991 commented Feb 22, 2024

Closes: #979
Closes: #1078

Integration-tests: informalsystems/basecoin-rs#165

Description


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests.
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

Copy link

codecov bot commented Feb 22, 2024

Codecov Report

Attention: Patch coverage is 68.16940% with 233 lines in your changes are missing coverage. Please review.

Project coverage is 66.52%. Comparing base (e048e34) to head (5ea1f9b).

Files Patch % Lines
...lients/ics07-tendermint/src/client_state/common.rs 19.62% 127 Missing ⚠️
...nts/ics07-tendermint/src/client_state/execution.rs 69.84% 76 Missing ⚠️
...ics07-tendermint/src/client_state/update_client.rs 89.18% 12 Missing ⚠️
.../ics07-tendermint/src/client_state/misbehaviour.rs 90.90% 9 Missing ⚠️
...core/ics24-host/cosmos/src/validate_self_client.rs 0.00% 5 Missing ⚠️
...ts/ics07-tendermint/src/client_state/validation.rs 95.78% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1097      +/-   ##
==========================================
- Coverage   66.53%   66.52%   -0.02%     
==========================================
  Files         204      208       +4     
  Lines       20532    20705     +173     
==========================================
+ Hits        13661    13773     +112     
- Misses       6871     6932      +61     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@seanchen1991 seanchen1991 changed the title Sean/detach client state verifier Detach client state verifier Feb 22, 2024
@seanchen1991 seanchen1991 marked this pull request as ready for review February 22, 2024 20:24
Copy link
Member

@Farhad-Shabani Farhad-Shabani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're almost there, just one more step to go! 🙌

The standalone functions currently insist on getting a client_state argument of the ClientState type (like here), making them practically unusable for developers who want to use a custom Tendermint Verifier.
In the custom route, they'd need to define their own newtype wrapper and implement all the ClientState traits on that. Therefore, they should be able to pass their own ClientState type to these functions. Though, there is a simpler way around: If we switch the type of the client_state field to ClientStateType, the problem is solved!

@seanchen1991
Copy link
Contributor Author

If we switch the type of the client_state field to ClientStateType, the problem is solved

That means we'll need to implement the TmVerifier trait on the ClientState type in the ics07-tendermint-types crate. Is that acceptable?

@Farhad-Shabani
Copy link
Member

That means we'll need to implement the TmVerifier trait on the ClientState type in the ics07-tendermint-types crate. Is that acceptable?

Good point! Upon reflection, I realized that we don't necessarily need to implement TmVerifier on a ClientState object. Since it doesn't consume anything from self. Therefore what about defining a unit DefaultVerifier struct and adding an additional argument as verifier: &impl TmVerifier to the verify_header and verify_misbehaviour functions? (Then, we should be able to pass a client state with the type of CientStateType to the standalone functions)
It would look like something as follows:

pub struct DefaultVerifier;

impl TmVerifier for DefaultVerifier {
    type Verifier = ProdVerifier;

    fn verifier(&self) -> Self::Verifier {
        ProdVerifier::default()
    }
}


impl<V> ClientStateValidation<V> for ClientState
where
    V: ClientValidationContext + TmValidationContext,
    V::AnyConsensusState: TryInto<TmConsensusState>,
    ClientError: From<<V::AnyConsensusState as TryInto<TmConsensusState>>::Error>,
{
    fn verify_client_message(
        &self,
        ctx: &V,
        client_id: &ClientId,
        client_message: Any,
    ) -> Result<(), ClientError> {
        verify_client_message(self.inner(), ctx, client_id, client_message, &DefaultVerifier)
    }
}


pub fn verify_client_message<V>(
    client_state: &ClientStateType, // now we should be able to use `CientStateType` here
    ctx: &V,
    client_id: &ClientId,
    client_message: Any,
    verifier: &impl TmVerifier,
) -> Result<(), ClientError>
where
    V: ClientValidationContext + TmValidationContext,
    V::AnyConsensusState: TryInto<TmConsensusState>,
    ClientError: From<<V::AnyConsensusState as TryInto<TmConsensusState>>::Error>,
{
    match client_message.type_url.as_str() {
        TENDERMINT_HEADER_TYPE_URL => {
            let header = TmHeader::try_from(client_message)?;
            verify_header(client_state, ctx, client_id, &header, verifier)
        }
        TENDERMINT_MISBEHAVIOUR_TYPE_URL => {
            let misbehaviour = TmMisbehaviour::try_from(client_message)?;
            verify_misbehaviour(client_state, ctx, client_id, &misbehaviour, verifier)
        }
        _ => Err(ClientError::InvalidUpdateClientMessage),
    }
}

@Farhad-Shabani Farhad-Shabani changed the title Detach client state verifier feat: detach client state verifier enabling users to implement custom verifier Feb 29, 2024
Copy link
Member

@Farhad-Shabani Farhad-Shabani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big thanks for tackling this issue @seanchen1991!
I did some tidying up. Plus, the client_state.rs file was getting pretty massive, making it tricky to move around. While we were pulling out implementations into standalone functions, I figured it was a good opportunity to organize the code better. So, I split the trait implementations into their own modules. Hope that makes sense to you too!

@Farhad-Shabani Farhad-Shabani merged commit 6dd3c64 into main Feb 29, 2024
15 checks passed
@Farhad-Shabani Farhad-Shabani deleted the sean/detach-client-state-verifier branch February 29, 2024 15:17
@Farhad-Shabani Farhad-Shabani added this to the 0.51.0 milestone Mar 12, 2024
Farhad-Shabani added a commit that referenced this pull request Sep 9, 2024
… verifier (#1097)

* Add empty Verifier trait stub

* Remove verifier field from ClientState

* Implement TmVerifier for ClientState

* Move ClientState trait methods into being standalone functions

* Fix some typos

* Remove unused fn param on update_client_state_on_misbehaviour

* Call standalone functions in trait implementations

* Move update_client and misbehaviour methods out of the ClientState impl

* Define `DefaultVerifier` type

* Replace `ClientState` fn params with `ClientStateType`

* Migrate ClientState::initialise function param back to ClientState

* Change some associated type constraints

* Change From<TmConsensusState> constraint to From<ConsensusStateType>

* impl From<ConsensusStateType> for AnyConsensusState

* impl some Displays when use-substrate feature is not enabled

* Remove unused displaydoc use statement

* Revert "Remove unused displaydoc use statement"

This reverts commit 92e8e36.

* Revert "impl some Displays when use-substrate feature is not enabled"

This reverts commit 083399c.

* Add docstring comment illustrating how to implement a custom verifier

* Ad unclog entry

* fix: set NIGHTLY_VERSION=nightly-2024-02-24

* imp: reorganize client_state.rs + clean-ups

* nit

---------

Co-authored-by: Farhad Shabani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
2 participants