Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Refactor Transaction through runtime & primitives #92

Closed
gavofyork opened this issue Mar 12, 2018 · 4 comments
Closed

Refactor Transaction through runtime & primitives #92

gavofyork opened this issue Mar 12, 2018 · 4 comments

Comments

@gavofyork
Copy link
Member

gavofyork commented Mar 12, 2018

The primitives crate is a dependency of the runtime crate. Yet the Transaction type defined in primitives, semantically depends on the contents of runtime since it expresses all callable endpoints within runtime in a strongly typed manner.

This combination has a number of problematic side effects:

  • When new end-points are added, they are added not just to their native crate, but also to one if its dependencies, making no sense.
  • It's not enough to publicly export any types by a runtime module and exposed through a callable endpoint. You need to actually move that type up into the primitives crate. If the type has impl logic, then that must be moved too (even if it's highly specialised and not very "primitive") or some other acrobatics used to circumvent.

Furthermore, requiring a strongly-typed dispatch at all implies another facepalm: Some endpoints themselves proxy a further dispatchable "proposal", causing a self-reference that means a bare type in the enum cannot be used and further allocations are needed. i.e.

enum Proposal {
    ...
    StartPublicReferendum(Proposal, Format),
    ...
}

must become StartPublicReferendum(Box<Proposal>, Format),.

Aside from these specific side-effects which are cause pain right now, the general ramifications of this leaving this unfixed are a tendency towards spaghetti references and monolithic code.

There are three ways of going that I can see:

1a. Move Transaction (and all that depend on it, like Block) to runtime. This would keep the current types as they are, but leave primitives to be just the super-low-level types and runtime to be the crate to be imported if high-level typing was needed.
1b. Move Transaction (and all that depend on it, like Block) to some other module (e.g. highlevel). This would keep the current types as they are, but leave primitives to be just the super-low-level types. highlevel would depend on runtime and be the crate to be imported.
2. Avoid making Transaction typed around any runtime-dependent information. Transaction would be more like in Ethereum where the dispatch element is just a byte blob to be interpreted at (or just before) the time of dispatch, not when the transaction is being initially deserialised. This fixes all problems including the Proposal-within-a-Proposal issue.

My preference is for option 2, moving away from this attempt to bake the dispatch logic into the type system, which seems to be forcing such problems on us. Aside from the great view from the ivory tower, I see no great need to represent the dispatch data under strong types prior to the time of dispatch.

CC @rphmeier

@gavofyork gavofyork added this to To Do in Runtime via automation Mar 12, 2018
@rphmeier
Copy link
Contributor

When new end-points are added, they are added not just to their native crate, but also to one if its dependencies, making no sense.

The rationale for this is based on the idea that the runtime implementation is a user of these transaction types as opposed to their owner. We can imagine a tree

                   ----- runtime
                 /
primitives <--- X
                 \
                   ----- stake-wallet-app

for example.

That said, I don't think it's terrible to have crates which are meant to interact with the runtime just link to it and import the types.

What I would really like to avoid is any kind of partial deserialization where we just pass bytes into submodules over and over again.

StartPublicReferendum(Box<Proposal>, Format)

IMO this is actually the correct way to express this relationship in Rust. The alternative is to replace the Box<Proposal> with a Vec<u8> and document that the member must be a correctly serialized proposal (note: this doesn't actually reduce allocations or overhead).

We should steer away from types which can't be resolved at deserialization time in general. Although in this specific case we might just want to consider splitting off StartPublicReferendum from the rest of the proposals: I don't think it's desired behavior to be able to create an unbounded chain of referenda to be started but rather just a single one.

It should be possible to create a generic solution that handles the sub-dispatch to modules.

@gavofyork
Copy link
Member Author

The real dependency graph is actually this:

                         runtime
                         +-------------------------------------+
                         |                                     |
                         |  governance module ++               |
                         |                     |               |
                         |  staking module     |               |
   primitives <----------+                     +-+ dispatcher  |  <-------+ Transaction  <------+  Native Auxilliaries
                         |  system module      |               |
                         |                     |               |
                         |  other modules...  ++               |
                         |                                     |
                         +-------------------------------------+

By placing Transaction in primitives you're entirely disregarding the semantic dependencies and that's why everything is so broken.

@gavofyork
Copy link
Member Author

gavofyork commented Mar 12, 2018

I understand your reticence (as a good and loyal rustacean) to use serialised data any more than the strict minimum. However, I fear this attitude is rather throwing the rest of the design under the bus for the sake of a fuzzy feeling that you're doing things by the book.

Option 2 would result in one fewer allocation (since you'd never need to fart around with Boxes at all). It might also reduce the complexity of any automated dispatch system since there would be no special cases for functions that accept Proposal.

@gavofyork
Copy link
Member Author

Although in this specific case we might just want to consider splitting off StartPublicReferendum from the rest of the proposals

This sentiment scares me a lot more than late deserialisation.

Runtime automation moved this from To Do to Done Mar 17, 2018
liuchengxu pushed a commit to subspace/substrate that referenced this issue Jun 3, 2022
helin6 pushed a commit to boolnetwork/substrate that referenced this issue Jul 25, 2023
* create SignedPayload struct

* align with generic extra updates

Co-authored-by: bwty <whalelephant@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants