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

Implement generic CLI query connection end #405

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thaodt
Copy link

@thaodt thaodt commented Aug 15, 2024

Part of #393.

Description

Following the guideline in issue description to do it.

This PR is still in-draft but I have 1 question for you @soareschen. I'm getting this error:

error[E0308]: mismatched types
  --> crates/cli/cli-components/src/impls/commands/queries/connection_end.rs:88:31
   |
49 | impl<App, Args, Build, Chain, Counterparty> CommandRunner<App, Args>
   |                        -----  ------------ expected type parameter
   |                        |
   |                        found type parameter
...
88 |         Ok(app.produce_output(connection_end))
   |                -------------- ^^^^^^^^^^^^^^ expected type parameter `Counterparty`, found type parameter `Chain`
   |                |
   |                arguments to this method are incorrect
   |
   = note: expected associated type `<Counterparty as hermes_relayer_components::chain::traits::types::connection::HasConnectionEndType<Chain>>::ConnectionEnd`
              found associated type `<Chain as hermes_relayer_components::chain::traits::types::connection::HasConnectionEndType<Counterparty>>::ConnectionEnd`
   = note: a type parameter was expected, but a different one was found; you might be missing a type parameter or trait bound
   = note: for more information, visit https://doc.rust-lang.org/book/ch10-02-traits.html#traits-as-parameters
   = note: an associated type was expected, but a different one was found
note: method defined here
  --> crates/cli/cli-components/src/traits/output.rs:10:8
   |
10 |     fn produce_output(&self, value: Value) -> Self::Output;
   |        ^^^^^^^^^^^^^^

I guess it relates to one of traits I used in Chain & Counterparty when implementing for RunQueryConnectionEndCommand but I still didn't figure out what's wrong in my implementation. Can you please point it out for me? Thanks.

Copy link
Collaborator

@soareschen soareschen left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I have added notes on how you can fix the errors.

for RunQueryConnectionEndCommand
where
App: CanLoadBuilder<Builder = Build>
+ CanProduceOutput<Counterparty::ConnectionEnd>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The connection end returned from CanQueryConnectionEnd is of type Chain::ConnectionEnd. So you need to use that to produce the CLI output.

+ CanRaiseError<String>,
Build: CanBuildChain<0, Chain = Chain> + HasChainTypeAt<1, Chain = Counterparty>,
Chain: CanQueryChainHeight + CanQueryConnectionEnd<Counterparty>,
Counterparty: HasHeightType + HasChainId + HasConnectionEndType<Chain>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

All the constraints for Counterparty are not really needed, thus can be removed.

+ CanParseArg<Args, symbol!("height"), Parsed = Option<Chain::Height>>
+ CanRaiseError<Build::Error>
+ CanRaiseError<Chain::Error>
+ CanRaiseError<String>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We didn't raise any string error in this implementation. So the constraint can be removed.

+ CanProduceOutput<Counterparty::ConnectionEnd>
+ CanParseArg<Args, symbol!("chain_id"), Parsed = Chain::ChainId>
+ CanParseArg<Args, symbol!("connection_id"), Parsed = Chain::ConnectionId>
+ CanParseArg<Args, symbol!("height"), Parsed = Option<Chain::Height>>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to add the following wiring to HermesParserComponents to enable the parsing of the given fields.

        (QueryConnectionEndArgs, symbol!("chain_id")): ParseFromString<ChainId>,
        (QueryConnectionEndArgs, symbol!("connection_id")): ParseFromString<ConnectionId>,
        (QueryConnectionEndArgs, symbol!("height")): ParseFromOptionalString<Height>,

@@ -57,7 +54,7 @@ impl CommandRunner<HermesApp> for QueryCommands {
match self {
Self::Client(cmd) => app.run_command(cmd).await,
Self::Clients(cmd) => cmd.run(app).await,
Self::Connection(cmd) => cmd.run(app).await,
Self::Connection(cmd) => app.run_command(cmd).await,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to add the following wiring to HermesCommandRunnerComponents to forward the subcommand to the corresponding runners:

        QueryConnectionSubCommand: RunQueryConnectionSubCommand,
        QueryConnectionEndArgs: RunQueryConnectionEndCommand,

height: Option<String>,
}

impl<App, Args, Build, Chain, Counterparty> CommandRunner<App, Args>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The following constraints should be added to CanUseHermesApp to check that the HermesApp context indeed implements CommandRunner for the corresponding command args:

    + CanRunCommand<QueryConnectionSubCommand>
    + CanRunCommand<QueryConnectionEndArgs>

@@ -1,6 +1,3 @@
mod connection;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The corresponding files at commands/query/connection should be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants