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 custom User-Agent header to all Hermes queries #3979

Closed
5 tasks
freak12techno opened this issue May 2, 2024 · 10 comments · Fixed by #4017
Closed
5 tasks

Add custom User-Agent header to all Hermes queries #3979

freak12techno opened this issue May 2, 2024 · 10 comments · Fixed by #4017
Assignees
Milestone

Comments

@freak12techno
Copy link
Contributor

Summary

Would be nice if all of the queries that are done by Hermes has their own specific User-Agent header.

Problem Definition

I have a few public nodes and when looking at their logs it's difficult to figure out what app is doing such requests.
There are a lot of queries like this:

45.10.26.95 - - [02/May/2024:16:53:11 +0200] "POST / HTTP/1.1" 200 226 "-" "tendermint.rs/0.34.0"

which I assume is Hermes, but if there's another Rust app that's doing such requests, it's impossible to distinguish the two.

Proposal

For all of the queries (to RPC node, LCD/REST node and not sure about gRPC node) append a custom User-Agent header (probably something like hermes/vX.Y.Z).

Acceptance Criteria


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@github-project-automation github-project-automation bot moved this to 🩹 Triage in Hermes May 2, 2024
@romac
Copy link
Member

romac commented May 22, 2024

This requires changes in tendermint-rs, to allow downstream users of HttpClient to specify the user agent.

Would you be open to create an issue there and maybe work on a PR for this?

@freak12techno
Copy link
Contributor Author

@romac done, see above.
As per whether I can work on this, not sure yet, but I can try.

@romac romac added this to the v1.9 milestone May 29, 2024
@ljoss17 ljoss17 modified the milestones: v1.9, v1.10 May 31, 2024
@freak12techno
Copy link
Contributor Author

@romac so in order to get this done, we need to update Hermes to tendermint-rs 0.37, but when I do this, I face a lot of issues and it fails to compile, namely this one:

error[E0631]: type mismatch in function arguments
   --> crates/relayer-types/src/core/ics26_routing/msgs.rs:146:30
    |
146 |                       .map_err(Error::malformed_message_bytes)?;
    |                        ------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected due to this
    |                        |
    |                        required by a bound introduced by this call
    |
   ::: crates/relayer-types/src/core/ics26_routing/error.rs:8:1
    |
8   | / define_error! {
9   | |     #[derive(Debug, PartialEq, Eq)]
10  | |     Error {
11  | |         Ics02Client
...   |
34  | |     }
35  | | }
    | |_- found signature defined here
    |
    = note: expected function signature `fn(tendermint_proto::error::Error) -> _`
               found function signature `fn(tendermint_proto::Error) -> _`
note: required by a bound in `std::result::Result::<T, E>::map_err`

Not sure how to resolve it, can you or someone assist here?

@romac
Copy link
Member

romac commented Jun 2, 2024

Ah we need to bump the dependency on tendermint-proto to v0.37.0 in ibc-proto and release that. Will do it next week.

@freak12techno
Copy link
Contributor Author

@romac ah, I didn't thought of that. I can make a PR bumping ibc-proto-rs's version of tendermint-proto to 0.37 (tried this locally, it indeed fixed it), do you mind?

@romac
Copy link
Member

romac commented Jun 2, 2024

Of course, go ahead, thanks!

@romac
Copy link
Member

romac commented Jun 3, 2024

I've just released ibc-proto v0.46.0, which now depends itself on tendermint-proto v0.37.0.

@freak12techno
Copy link
Contributor Author

@romac thanks!

One more question: it's almost done but I I want to pass Hermes version to the app, like this:

    let rpc_client = rpc::HttpClient::builder(addr.clone().try_into().unwrap())
        .user_agent(format!("hermes/{}", clap::crate_version!()))
        .build()
        .map_err(|e| Error::rpc(addr.clone(), e))?;

but apparently the clap module is not a part of the crates relayer and chain-registry, so it fails to compile if I try to use it:

error[E0433]: failed to resolve: use of undeclared crate or module `clap`
   --> crates/relayer/src/light_client/tendermint.rs:259:42
    |
259 |         .user_agent(format!("hermes/{}", clap::crate_version!()))
    |                                          ^^^^ use of undeclared crate or module `clap`

error[E0433]: failed to resolve: use of undeclared crate or module `clap`
   --> crates/relayer/src/chain/cosmos.rs:984:46
    |
984 |             .user_agent(format!("hermes/{}", clap::crate_version!()))
    |                                              ^^^^ use of undeclared crate or module `clap`

What do you think would be the best way to resolve it?

(relayer-cli already has clap module, so it's working there)

@romac
Copy link
Member

romac commented Jun 3, 2024

Let me take a look, I'll figure something out.

@romac
Copy link
Member

romac commented Jun 3, 2024

The issue here is that the ibc-relayer and chain-registry crates are versioned differently (0.x.y) from the ibc-relayer-cli (1.x.y) crate, and we have no way to get the version of the ibc-relayer-cli crate from the ibc-relayer crate.

I would suggest adding

pub const HERMES_VERSION: &str = "1.8.3";

in crates/relayer/src/lib.rs and use HERMES_VERSION instead of clap::crate_version!().

We'll make sure we keep that constant up to date when we bump the Hermes version.

@github-project-automation github-project-automation bot moved this from 🩹 Triage to ✅ Done in Hermes Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

3 participants