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

Tracking issue: Modularize the Penumbra application code into coherent crates #2288

Closed
34 tasks done
hdevalence opened this issue Mar 31, 2023 · 6 comments
Closed
34 tasks done

Comments

@hdevalence
Copy link
Member

hdevalence commented Mar 31, 2023

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

Since there isn't already a Rust application framework for building Tendermint chains, we've had to build one at the same time as we've built out our application. This has been a messy and iterative process, and along the way, we've passed from having separate crates for separate components of the application (which turned out not to be entirely separate) to having effectively all of our application logic in a single crate, formerly called penumbra_component and now called penumbra_app (after #2282).

However, while looking at the current structure of our code, we realized there may be a path to re-modularizing it, not in a single stop-the-world refactor, but gradually peeling out chunks of functionality.

Describe the solution you'd like

The following plan is a sketch; the first steps are concrete and definitely a good idea, while the later steps are more speculative. As we do this, let's think step by step, and notice any unexpected frictions.

  • Change the Component trait so that the AppState passed to init_chain is an associated type.

    This makes the definition of the Component trait independent of the rest of the Penumbra code. Later, it will allow us to split up the genesis state into per-component chunks, with each different Component operating on its own definition of its genesis data. This avoids cross-component dependencies (e.g., component X has to know about component Y because the genesis data passed to component X has types from component Y).

    It might be worth delaying this until we can clean up the IBC code, which has a number of Component implementations that may not be necessary.

  • Change the ActionHandler trait to add a CheckStatelessContext: Clone + Send + Sync + 'static associated type.

    This makes the definition of the ActionHandler trait independent of the rest of the Penumbra code. Most ActionHandlers don't make use of the context, and could have CheckStatelessContext = (). Others that do probably just use the anchor, and could have that specified as the verification context, rather than a full Arc<Transaction>. This avoids cross-action dependencies (e.g., every action has to know about every other action, because it has to know about the Transaction type that contains all of them including itself).

    Longer-term, this also presents some interesting opportunities. For instance, we could use the tower-batch strategy to create a context struct that had channels communicating with tower::Services performing batch verification of signatures and proofs.

  • Pull the revised Component and ActionHandler traits out into a new penumbra_component crate.

    Once the traits don't depend on external traits, they can be extracted into a "bottom-level" penumbra_component crate that only depends on the penumbra_storage crate, and no other Penumbra workspace crates. Ideally, the tower_abci + penumbra_storage + penumbra_component stack would be useful to third party teams building Rust blockchains.

  • Peel components out of penumbra_app into individual crates, bottom-up

    Once the Component and ActionHandler traits are in a bottom-level penumbra_component crate with no Penumbra-specific dependencies, we can peel components out into individual crates, bottom-up. Some initial refactors might be:

    • moving the sct and compact_block "pseudo-components" into penumbra_chain, which can now depend on penumbra_component;
    • extracting a penumbra_shielded_pool crate with the shielded pool component;
    • extracting a penumbra_ibc crate with the IBC component.
      This will force us to declare each component's public APIs and the dependency relations between components. Along the way, we may discover dependency cycles among some of the higher-level components, which we'll need to resolve. The structure we're aiming for is one where we have a top-level penumbra_app crate depending on its individual components, each in separate crates containing the Component impl, and ActionHandlers for relevant messages.
      Updated subchecklist:
    • Extract penumbra-shielded-pool
    • Extract penumbra-ibc
    • Extract penumbra-dao
    • Extract penumbra-stake, including
      • Definitions of Delegate, Undelegate, UndelegateClaim, ValidatorDefinition actions (move the IsAction impls to transaction/src/is_action.rs for now)
      • The component logic itself, in a top-level component module feature-gated by a component feature;
      • The action handlers, which should move out of app into component/action_handler/
      • Metrics code in a private component::metrics module, with register_metrics publicly re-exported from component
      • State keys in component::state_key.
      • A top-level crate::event module (can be empty if we don't have events defined)
      • All imports patched back up again, including changing the transaction crate to update the location of the actions
    • Extract penumbra-dex
      • Definitions of Swap, SwapClaim, Position* actions (move the IsAction impls to transaction/src/is_action.rs for now)
      • The component logic itself, in a top-level component module feature-gated by a component feature;
      • The action handlers, which should move out of app into component/action_handler/
      • Metrics code in a private component::metrics module, with register_metrics publicly re-exported from component
      • State keys in component::state_key.
      • A top-level crate::event module (can be empty if we don't have events defined)
      • All imports patched back up again, including changing the transaction crate to update the location of the actions
    • Extract penumbra-governance
      • Definitions of Proposal*, ValidatorVote, DelegatorVote actions (move the IsAction impls to transaction/src/is_action.rs for now)
      • The component logic itself, in a top-level component module feature-gated by a component feature;
      • The action handlers, which should move out of app into component/action_handler/
      • Metrics code in a private component::metrics module, with register_metrics publicly re-exported from component
      • State keys in component::state_key.
      • A top-level crate::event module (can be empty if we don't have events defined)
      • All imports patched back up again, including changing the transaction crate to update the location of the actions
  • Align protobuf package organization with new Rust code organization

    All of the preceding moves only affect Rust code, but leave the proto packages unchanged. Once we have a coherent organization of the Rust code, we can update the proto packages to mirror it. This also allows us to define per-component GRPC methods.

  • Align gRPC services to match new crate-aligned proto organization - ibc: split up query rpc implementations #3079

  • Lift functionality out of the penumbra_crypto crate into component crates.

    Similar to the penumbra_app crate, the penumbra_crypto crate has likewise become a catch-all for everything not involving the state machine. But once we have components in individual crates, we can lift functionality out of the penumbra_crypto crate, this time working top-down. An initial refactor might be lifting the dex module up to the penumbra_dex crate. Eventually, at the bottom, it would be nice to have Notes defined as part of the penumbra_shielded_pool crate.

    At the end, we'd end up with a stub penumbra_crypto that just had the key hierarchy and some core numeric types like Amount or U128x128, which could potentially be renamed penumbra_keys or something. Each component crate would have the structures and Actions relevant to that functionality, aligned with the corresponding proto package, together with any proof statements for those actions and the application-side logic implementing the Component and ActionHandler traits. To make this work, we'd need each crate to have a component-impl (or similar) Cargo feature that would feature-gate the server-side logic and async execution, allowing client code to import the data structure definitions and client logic without having to pull in Tokio and other dependencies that might not be desirable everywhere (e.g., wasm). It might also be useful to feature-gate the proofs behind a proof-impl Cargo feature for similar reasons.

@hdevalence
Copy link
Member Author

One potential problem: currently, most of the Action messages are defined outside of the penumbra_app crate where the impls are defined. So if we extract the ActionHandler trait into a penumbra_component crate, we won't be able to leave the impls where they are -- we'd either have to skip all the way to the end (doing a giant, stop-the-world refactor), or introduce a shim newtype in the penumbra_app crate, like struct SpendWrapper<'a>(&'a Spend). The latter might not be so terrible as a temporary measure?

@zbuc zbuc added this to Testnets Mar 31, 2023
@zbuc zbuc moved this to Future in Testnets Mar 31, 2023
@conorsch conorsch moved this from Future to Tracking Issues in Testnets Apr 7, 2023
@conorsch conorsch changed the title Modularize the Penumbra application code into coherent crates Tracking issue: Modularize the Penumbra application code into coherent crates Apr 7, 2023
hdevalence added a commit that referenced this issue Apr 29, 2023
part of #2288

later, it'd be cool to have each component define its own isolated state and have the penumbra-chain crate stitch them together
hdevalence added a commit that referenced this issue Apr 29, 2023
part of #2288

later, it'd be cool to have each component define its own isolated state and have the penumbra-chain crate stitch them together
hdevalence added a commit that referenced this issue Apr 29, 2023
Part of #2288.  This is a proof-of-possibility for the architecture described
in that issue.  Now, the `penumbra-shielded-pool` crate is self-contained, with
both the on-chain structures required to track the shielded pool, and the
transaction actions needed to interact with it.

In the future, a fair amount of functionality currently in the
`penumbra-crypto` crate could now be lifted up into this crate, for instance
the definitions of the spend and output proof statements.
hdevalence added a commit that referenced this issue Apr 29, 2023
Part of #2288.  This is a proof-of-possibility for the architecture described
in that issue.  Now, the `penumbra-shielded-pool` crate is self-contained, with
both the on-chain structures required to track the shielded pool, and the
transaction actions needed to interact with it.

In the future, a fair amount of functionality currently in the
`penumbra-crypto` crate could now be lifted up into this crate, for instance
the definitions of the spend and output proof statements.
@conorsch conorsch added this to the Security audit milestone May 4, 2023
@conorsch
Copy link
Contributor

Align protobuf package organization with new Rust code organization

Achieved in #3077 There's a bit of follow up tracked in #3078

@conorsch
Copy link
Contributor

Closing as complete!

@github-project-automation github-project-automation bot moved this from Tracking Issues to Testnet 61: Dione in Testnets Sep 22, 2023
@hdevalence hdevalence reopened this Sep 22, 2023
@hdevalence
Copy link
Member Author

Reopening, we also need to split into per component RPC implementations. I'll do that today.

conorsch added a commit to penumbra-zone/osiris that referenced this issue Sep 25, 2023
Updates the import paths for the `penumbra_proto` crate for
compatibility with Testnet 61 Dione [0], specifically the
work tracked in [1].

[0] penumbra-zone/penumbra#3046
[1] penumbra-zone/penumbra#2288
@conorsch conorsch added stale Issue may no longer be relevant, needs triage and removed stale Issue may no longer be relevant, needs triage labels Oct 6, 2023
@conorsch
Copy link
Contributor

conorsch commented Oct 6, 2023

Added the follow-up issue #2914 to the checklist at the top. Once 2914 is done, we can close out this tracking issue.

@hdevalence
Copy link
Member Author

I don't think #2914 is related to this issue, it's definitely good to do but it's not part of modularizing our own code.

@github-project-automation github-project-automation bot moved this from Testnet 61: Dione to Testnet 63: Rhea (Web Wallet) in Testnets Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Testnet 63: Rhea (Web Wallet)
Development

No branches or pull requests

2 participants