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

Auto-generate hooks based on Runtime #320

Open
preston-evans98 opened this issue May 23, 2023 · 4 comments
Open

Auto-generate hooks based on Runtime #320

preston-evans98 opened this issue May 23, 2023 · 4 comments

Comments

@preston-evans98
Copy link
Member

Currently, users have to manually define their TxHooks implementation based on the modules they use. In principle, though, it seems like it should be possible (and much less error-prone) to automatically generate the hooks based on the runtime.

cc @bkolad

@bkolad
Copy link
Member

bkolad commented May 25, 2023

What about this:

  1. We introduce ModuleHook trait with pre_dispatch_tx_hook, post_dispatch_tx_hook etc methods
  2. Every module that introduces hooks needs to implement this trait.
  3. Runtime gets an additional macro and [hooks] attribute:
#[derive(Genesis, DispatchCall, MessageCodec, Hooks)]
pub struct Runtime<C: Context> {
   #[hook]
    pub sequencer: sequencer::Sequencer<C>,

    pub bank: bank::Bank<C>,

    pub election: election::Election<C>,

    pub value_setter: value_setter::ValueSetter<C>,

   #[hook]
    pub accounts: accounts::Accounts<C>,
}

And the implementation of the Hook trait is automatically derived.

@preston-evans98
Copy link
Member Author

This looks great overall. Do we have a motivating a use case where users would want a module without its hooks though? If not, can we just derive a no-op Hook trait implementation for all Modules and then get rid of the #[hook] annotation here? I'm worried that forgetting to add the annotation will be a very subtle way to completely break security guarantees

@bkolad
Copy link
Member

bkolad commented May 26, 2023

Yeah we can automatically derive implementations with noop.

@bkolad
Copy link
Member

bkolad commented Jun 1, 2023

After incorporating #361, the hooks implementation has a well-defined structure, allowing us to implement the macro. However, there is a small remaining issue.

impl<C: Context> TxHooks for Runtime<C> {
    type Context = C;

    fn pre_dispatch_tx_hook(
        &self,
        tx: Transaction<Self::Context>,
        working_set: &mut WorkingSet<<Self::Context as Spec>::Storage>,
    ) -> anyhow::Result<<Self::Context as Spec>::Address> {
        self.accounts.pre_dispatch_tx_hook(tx, working_set)
    }

    fn post_dispatch_tx_hook(
        &self,
        tx: &Transaction<Self::Context>,
        working_set: &mut WorkingSet<<Self::Context as Spec>::Storage>,
    ) -> anyhow::Result<()> {
        self.accounts.post_dispatch_tx_hook(tx, working_set)
    }
}

The issue with the pre_dispatch_tx_hook method is that it returns an Address type, which is not ideal for macros. As we add more modules, we will need to handle the return types for pre_dispatch_tx_hook of each module. One possible solution is to generate a type that holds all the types, but it is unclear how the user would interact with this generated type.

To address this, we can modify the pre_dispatch_tx_hook method to return Result<()>. However, we still need to obtain the Account from the PublicKey. There are two ways to solve this:

1.Remove the functionality that updates the Address corresponding to a given PublicKey and use PublicKey::default_address() everywhere.
2. Send both the Address and PublicKey together in a Transaction. In this case, the sov-accounts module would validate if they match and return Result<()>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants