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

Design a mechanism to allow dynamic AppHandlers in the IBC component #3127

Closed
Tracked by #3125
hdevalence opened this issue Sep 30, 2023 · 3 comments · Fixed by #3245
Closed
Tracked by #3125

Design a mechanism to allow dynamic AppHandlers in the IBC component #3127

hdevalence opened this issue Sep 30, 2023 · 3 comments · Fixed by #3245
Assignees
Labels
A-IBC Area: IBC integration with Penumbra

Comments

@hdevalence
Copy link
Member

hdevalence commented Sep 30, 2023

Currently, penumbra-ibc depends on penumbra-shielded-pool, because the penumbra-ibc crate contains the implementation of the ICS20 token transfer handler, which edits the shielded pool state. This isn't the design we want, though, because we want the penumbra-ibc crate to be a "foundational" crate like penumbra-storage that's also usable by other projects. #3125 brings this problem from the backburner to the front burner.

Packet handling in penumbra-ibc is performed by the AppHandler trait, which exposes a generic interface for custom behavior. But currently, this is hardcoded to use an implementation that does Penumbra-specific ICS20 handling. Instead, we need a way to allow a dependency to implement the AppHandler, and then somehow cause that app handler to be used by the penumbra-ibc code.

This may also mean the AppHandler trait needs to be object-safe, and that may cause issues.

Two ideas on how we could do this:

  1. We could pass some kind of configuration object with the AppHandler implementation in somewhere. This may be difficult, because the existing Component interface used by penumbra-ibc may not have a way to pass in extra parameters.
  2. We could use global state, and have a registration mechanism. Dependencies would register app handlers at startup, and those handlers would be used during execution.

The downside of (2) is that it wouldn't be possible to have two different applications both using penumbra-ibc to do different things inside of the same process. This is kind of aesthetically displeasing, but I'm not sure it's a real problem at the moment, since it seems unlikely someone would, e.g., import all of Penumbra and all of Astria into one program. Otherwise, it seems potentially simpler.

In a previous iteration of informal design discussions with @avahowell about this issue, I think we'd felt that (2) was probably the right option, but we should compare both approaches.

We'll also need to design/refine the external interface, now that the AppHandler will be public.

@hdevalence
Copy link
Member Author

@noot To provide context for this design work, it would be helpful to know about how Astria works, in particular, how Astria represents accounts and balances, and what Astria's ICS20 handler would need to do in order to record the results of token transfers.

@hdevalence
Copy link
Member Author

  1. We could pass some kind of configuration object with the AppHandler implementation in somewhere. This may be difficult, because the existing Component interface used by penumbra-ibc may not have a way to pass in extra parameters.

Another thought on this (cf #3126): we could make the penumbra-ibc code not implement the Component trait directly, but instead implement its own trait, like ComponentWithHandlerConfig or something. This new trait would have almost identical methods, but with the addition of an extra parameter that would provide the dyn AppHandlers.

Then the downstream code that defines the AppHandler could provide a Component impl that would forward to the methods on ComponentWithHandlerConfig but passing in the correct app handlers.

@noot
Copy link
Collaborator

noot commented Oct 6, 2023

@hdevalence Astria represents accounts and balances as just a key-value pair in state, where the key is accounts/balance/<ADDRESS>. For example, the transfer cod e here (https://github.com/astriaorg/astria/blob/main/crates/astria-sequencer/src/accounts/action.rs#L49) shows balance being updated in accounts. See the state_ext trait here for how balances (and nonces) for accounts are stored in state: https://github.com/astriaorg/astria/blob/main/crates/astria-sequencer/src/accounts/state_ext.rs

We only have support for the native token right now - for extending this to tokens, I imagine we can do something like changing the storage key to accounts/<TOKEN_ID>/balance/<ADDRESS>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-IBC Area: IBC integration with Penumbra
Projects
Archived in project
Status: Testnet 63: Rhea (Web Wallet)
Development

Successfully merging a pull request may close this issue.

5 participants