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

relayer: add support for penumbra #4292

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

erwanor
Copy link
Contributor

@erwanor erwanor commented Jan 26, 2025

Hi folks, for a little over year we have been maintaining our own fork of the hermes relayer software over at https://github.com/penumbra-zone/hermes.

One major block to upstreaming Penumbra support has been to get our crate workspace published and resolve the tree of unpublished dependencies that came with. This has been done and we would like to add this capability (relaying to Penumbra chains) to hermes.

At a high-level, the changes in this PR are as follow:

  • extending ChainConfig with a new variant. 1
  • a PenumbraChain implementation
  • a well contained refactor of chain::client_settings::settings
  • adds some missing conversion traits for IBC query domain types

Our goal is to progressively extend the end-to-end testing that CosmosSdk chains benefit from.


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
    • If guide has been updated, tag GitHub user mircea-c
  • 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).

Footnotes

  1. Follow-up from our work from a year ago: Change config format to scope configs by type #3644

erwanor and others added 2 commits January 25, 2025 20:49
Co-Authored-By: Henry de Valence <[email protected]>
Co-Authored-By: Ava Howell <[email protected]>
Co-Authored-By: Chris Czub <[email protected]>
Co-Authored-By: Conor Schaefer <[email protected]>
Co-Authored-By: noot <[email protected]>
Co-Authored-By: Henry de Valence <[email protected]>
Co-Authored-By: Ava Howell <[email protected]>
Co-Authored-By: Chris Czub <[email protected]>
Co-Authored-By: Conor Schaefer <[email protected]>
Co-Authored-By: noot <[email protected]>
@erwanor erwanor marked this pull request as ready for review January 26, 2025 03:58
@erwanor erwanor marked this pull request as draft January 26, 2025 04:00
Co-Authored-By: Henry de Valence <[email protected]>
Co-Authored-By: Ava Howell <[email protected]>
Co-Authored-By: Chris Czub <[email protected]>
Co-Authored-By: Conor Schaefer <[email protected]>
Co-Authored-By: noot <[email protected]>
@erwanor erwanor marked this pull request as ready for review January 26, 2025 04:50
@ljoss17
Copy link
Contributor

ljoss17 commented Jan 27, 2025

Hi @erwanor
Thank you for the work, this looks great!

I understand that goal to progressively extend the end-to-end testing for Penumbra, but I feel it would still be required to run at least the transfer test before merging the Penumbra compatibility.
This would require something similar to what has been done to bootstrap consumer chains or Namada chain:

This would mainly focus on updating the test-framework crate, more specifically something like this

pub fn bootstrap_namada_node(

Once this is setup it is possible to easily disable individual tests using a feature, e.g. https://github.com/informalsystems/hermes/blob/master/tools/integration-test/src/tests/mod.rs#L13

@hdevalence
Copy link
Contributor

I understand that goal to progressively extend the end-to-end testing for Penumbra,

Sorry, to clarify, this is not the goal. The goal is to mend the fractures in the IBC ecosystem that arose from past coordination issues. Our goal is to avoid having Informal (who operates relayers to Penumbra) have to run and maintain a bespoke fork of Hermes which existed for no good reason. To that end, we would prefer to get the fork reconciled as soon as possible -- since this code is already running in prod -- and improve the code over time, rather than continuing to generate additional, unnecessary work for everyone while that process completes.

@mpoke
Copy link

mpoke commented Jan 29, 2025

Hi @hdevalence @erwanor. What’s the timeline on this from your side?

We discuss it internally and we can go ahead with merging this PR without additional tests (especially given that the code is already running in production). We propose the following plan:

  • We first need to wait for the support for Namada to be merged as this process started a while ago and therefore has priority (we want to avoid conflicts that well add further delays to that work). We estimate that this will be done by end-of-week. This will result in a new Hermes release.
  • Next week, we can review the current PR and merge it to main. Then, we can cut a new release of Hermes that will have Penumbra support. We plan to mention though in the release notes that the code didn’t go through the standard testing procedure and additional tests will be added in the future. We’ll also mention that the code was already run in production.

The goal is to mend the fractures in the IBC ecosystem that arose from past coordination issues.

We are very supportive of this goal. We’d love to further discuss how we can make this happen for both Hermes and ibc-rs. Let’s sync on this on a call.

@erwanor
Copy link
Contributor Author

erwanor commented Jan 30, 2025

Thanks for getting back, it's appreciated. I think this works, the timeline on our end is "as-soon-as-possible", so we will be on stand-by to follow-up asap.

We are very supportive of this goal. We’d love to further discuss how we can make this happen for both Hermes and ibc-rs. Let’s sync on this on a call.

Agreed, be in touch.

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.

4 participants