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

Rebase/Rebuild Penumbra Hermes support on top of backend-specific configs #3477

Closed
Tracked by #3125 ...
hdevalence opened this issue Dec 5, 2023 · 15 comments · Fixed by penumbra-zone/hermes#12
Closed
Tracked by #3125 ...
Assignees
Labels
_P-high High priority
Milestone

Comments

@hdevalence
Copy link
Member

Is your feature request related to a problem? Please describe.

Our initial attempt at Hermes support for Penumbra was done as a one-off, since Hermes didn't have support for non-Cosmos chains. Since then we landed informalsystems/hermes#3636 and can rebuild a Penumbra backend on top of new Hermes.

@conorsch
Copy link
Contributor

Haven't been able to get Hermes building against 64 yet. Here's what I tried:

  1. Bump penumbra-proto dep to v0.64.0, here: https://github.com/penumbra-zone/hermes/blob/d8c0245f05c065286d670f4b5c52be44fba8d4b2/crates/relayer/Cargo.toml#L78
  2. Observe build errors
  3. Rename IbcAction to IbcRelay, matching penumbra-ibc: rename ibc relay actions #3284
  4. Bump prost 0.11 -> 0.12.3
  5. Bump ibc-proto 0.32.1 -> 0.39.0
  6. Adjust traits where necessary

Even after all that, I'm stuck on trait problems. @avahowell can you give it a go? I recommend starting from step 1 above, on a clean branch of the hermes fork. Once we have a clean build against v0.64.0, then we can try another patch for the cnidarium rename.

@erwanor
Copy link
Member

erwanor commented Dec 15, 2023

@conorsch if i want to take a look, i should checkout that d8c0245 commit and run cargo build -r right?

@conorsch
Copy link
Contributor

@erwanor That's right! Don't forget to manually increment the penumbra-proto version number, to v0.64.1, which includes the cnidarium rename.

@hdevalence
Copy link
Member Author

This issue is for rebuilding Hermes support on top of backend-specific configs. We shouldn't be checking out random git commits and attempting to patch them up. We should be using the rewritten code. What branch does that live in?

@conorsch
Copy link
Contributor

The git commit referenced above is the current HEAD of main branch on the Penumbra fork of Hermes, post merge of penumbra-zone/hermes#11.

@hdevalence
Copy link
Member Author

Yes, that code is not to be maintained or developed going forward. This issue is tracking rebuilding Hermes support on top of the changes we merged upstream two months ago. What happened to that effort?

@conorsch
Copy link
Contributor

Yes, that code is not to be maintained or developed going forward.

I've set the penumbra-zone/hermes fork to archived/read-only, to make it clear we shouldn't be interacting with that codebase as the default. Last week we merged two (2) PRs into that fork:

Those changes are necessary but not sufficient for restoring Penumbra compatibility in Hermes, but our goal is to leave that codebase behind, and use it only as a reference. Next steps should be:

  1. Branch from upstream latest tag: https://github.com/informalsystems/hermes/releases/tag/v1.7.4
  2. Re-implement Penumbra support, targeting compat with v0.64.1
  3. Ensure that logic from PRs 10 & 11 is accounted for.
  4. PR Penumbra compatibility into upstream.

@hdevalence
Copy link
Member Author

No, we need to be using that repo to develop our Hermes integration. I unarchived it. The point of this issue is that we need to rebuild Penumbra support, in that repo, on top of the changes we were able to merge upstream in the issue linked above, so that the code is maintainable going forward.

@hdevalence
Copy link
Member Author

Not sure if it is worth aiming at but since #3531 landed it may be possible to have Penumbra Hermes support not use ABCI Query interface at all. we could perhaps at the astria relayer code for an example.

@conorsch
Copy link
Contributor

conorsch commented Jan 4, 2024

We discussed this at sync today. Right now, Hermes is working against Testnet 64, although it's still built against Testnet 63 dependencies. That's "good enough" for now, so we plan to hold steady with the same setup on Testnet 65. If things break, we'll prioritize rebasing at that point. For now, it's higher priority to work with Astria on their IBC needs.

Separately, it would be nice to have a Hermes instance running against preview, so we could monitor compatibility more closely.

@conorsch
Copy link
Contributor

conorsch commented Feb 2, 2024

Work is active on this front, see penumbra-zone/hermes#12. However, we don't intend to block release of 65 (#3554) in order to land these changes.

@conorsch
Copy link
Contributor

conorsch commented Feb 6, 2024

Update from IBC sync today: @avahowell is optimistic about having PR against upstream by Friday, 2024-02-09. We'd love to see it land upstream, but either way, once it's working, we can start using it again on Testnet 65. We'll miss go-live of 65, so let's aim to follow up with a Hermes deploy on or around Monday 2024-02-12.

@avahowell
Copy link
Contributor

avahowell commented Feb 7, 2024

Overview on the progress and remaining items here (pr: penumbra-zone/hermes#12):

  • Add a stub implementation of the penumbra ChainEndpoint, using the new multi chain backend and config
  • add a view service and custody service to the ChainEndpoint
  • implement the queries for ChainEndpoint
  • implement transaction building and submitting with the custody service
  • write configs and verify that relaying on penumbra devnets works
  • write configs and verify that relaying between penumbra and cosmos sdk works

@hdevalence hdevalence added the _P-high High priority label Feb 9, 2024
@aubrika aubrika added this to the Sprint 0 milestone Feb 13, 2024
@aubrika aubrika linked a pull request Feb 20, 2024 that will close this issue
@conorsch
Copy link
Contributor

Update from IBC sync today: @avahowell is optimistic about having PR against upstream by Friday, 2024-02-09.

Yet another update: @avahowell now expects a PR to be ready for review by this Friday, 2024-02-23. New commits landing the boxed gRPC services (#3711) in the Hermes code have been pushed to the WIP PR branch. The checklist above is still accurate: config changes are in, and the code compiles, but there's still active debugging towards getting relaying working between Penumbra devnet chains.

@avahowell
Copy link
Contributor

avahowell commented Feb 21, 2024

I was able to successfully test:

  • Opening channels between penumbra testnets
  • Relaying packets between penumbra testnets
  • Opening channels between cosmos and penumbra (using the celestia mocha-4 testnet and penumbra deimos testnet)
  • Relaying packets between mocha-4 and deimos

We should do a review pass on the code in penumbra-zone/hermes#12 in order to ensure that the approach taken will be maintainable going forward, both from a penumbra perspective (we're using the penumbra crates to build txs now) and a hermes perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
_P-high High priority
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants