chainflip-io / chainflip-backend

The Chainflip backend repo, including the Chainflip Node and CFE.
50 stars 14 forks source link

[CH-1444] Refactor Witnesser #302

Closed dandanlen closed 3 years ago

dandanlen commented 3 years ago

First mentioned in #214:

Currently the pattern for exposing witness calls is a bit convoluted: We need to add the Witness trait (implemented by palletwitness only) and the EnsureWitness trait to Config of the pallet whose calls we want to witness. Then we expose a witnesscall that delegates to one of the pallet's extrinsics. Edit: the witness_* delegator is optional but makes it much easier to use

I think it would be clearer if the witness_* extrinsics were defined inside pallet_witness. This way have all of these calls in one place and can get a quick overview of our "witnessing" api surface, and less noise in each of the pallets that actually implement the functionality. It might also make it more straightforward for client interaction (subxt) since we no longer have to interact directly with a bunch of different pallets.

I think the change is fairly simple on the state-chain, but would have an impact on the extrinsics api and require some refactoring on the CFE.

If we think this is a good idea I can write up a more details spec here and/or in clubhouse.

Implementation

There are currently only three witnessed transactions on the state chain (at least on the current develop branch), defined across two pallets:

These will be moved into the Witnesser pallet. The witnesser will need to include the target pallets as an explicit dependency in its Cargo.toml so that the helper extrinsics can delegate to those pallets.

The CFE will need to adapt its definitions to remain compatible.

Checklist:

morelazers commented 3 years ago

Does this break anything on the CFE? @kylezs

dandanlen commented 3 years ago

Does this break anything on the CFE? @kylezs

Think I can answer this - it likely means changing some of the call definitions, although it should be sufficient to just move them into a different module, should require anything to be re-written. (Will edit the issue appropriately)

morelazers commented 3 years ago

LGTM 👍