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

refactor(ibc): remove dependency on penumbra-chain specific state keys #3528

Closed
wants to merge 10 commits into from

Conversation

noot
Copy link
Collaborator

@noot noot commented Dec 20, 2023

This PR refactors penumbra-ibc to not use penumbra-chain, which has penumbra-specific storage methods for getting chain info. This will allow other chains to use the IBC implementation without restricting the format/location of their chain-specific storage.

In particular, the trait that needs to be implemented by the chain using the IBC impl is:

pub trait ChainStateReadExt: StateRead {
    async fn get_chain_id(&self) -> Result<String>;
    async fn get_revision_number(&self) -> Result<u64>;
    async fn get_block_height(&self) -> Result<u64>;
    async fn get_block_timestamp(&self) -> Result<tendermint::Time>;
}

In this PR, IBC functions which require these methods require a generic <S: ChainStateReadExt>, which shall be implemented on top of anything that implements StateRead.

stuff I would like feedback on:

  • the Component in penumbra-ibc no longer implements cnidarium_component::Component and the IbcRelayWithHandler type no longer implements ActionHandler, due to restrictions on the generic types on those traits.
  • I can (probably) make a ComponentWithChainStateRead/ActionHandlerWithChainStateRead that do implement Component/ActionHandler instead
  • the alternative to this is I update those traits to require a ChainStateReadExt bound for the required methods, but this unnecessarily requires changing the other implementations of the type that don't need that trait, which seems non-optimal.
  • I made a StateDeltaWrapper inside App which implements ChainStateReadExt (along with StateRead and StateWrite)
  • ideally I would be able to directly implementation ChainStateReadExt for StateDelta/Snapshot within the crate itself but both of those are external types, so wrapper is my best bet :(
  • I implemented derive macros for StateRead and StateWrite which the wrappers can use, these can also be used by users of penumbra-ibc
  • should the ChainStateReadExt live in cnidarium_component or somewhere else? (also thoughts on the naming for this)?

@hdevalence
Copy link
Member

I haven't yet looked through it in detail, but my instinct is that having to have a custom derive suggests that it's not the way to go. A custom derive makes it easier to make new types implementing the traits. But so far, we've built the entire storage API not around that, but instead around using extension traits to generically compose new behavior onto existing generic types. I'm interested in digging into this and seeing if we can apply a similar strategy here.

Comment on lines +6 to +11
pub trait ChainStateReadExt: StateRead {
async fn get_chain_id(&self) -> Result<String>;
async fn get_revision_number(&self) -> Result<u64>;
async fn get_block_height(&self) -> Result<u64>;
async fn get_block_timestamp(&self) -> Result<tendermint::Time>;
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should go in cnidarium-component, since this is really an IBC host chain interface, that should be named as such and go in the ibc crate, right?

@erwanor
Copy link
Member

erwanor commented Jan 16, 2024

Work in progress in #3598, thanks for starting this!

@erwanor erwanor closed this Jan 16, 2024
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.

Refactor penumbra-ibc to not use penumbra-chain
3 participants