On-demand integration modules

Fellows
1mo ago
3 Comments
Approved

This is about paying for the delivery of

https://collectives.subsquare.io/posts/16

Report here: https://collectives.subsquare.io/posts/20

Validation done so far:

  1. Requested amount matches initial amount asked.
  2. Contributions are there and seem to match the report.

Noteworthy the following have not yet been implemented/should be improved in a follow-up:

  1. Ancestry proofs. Right now the on-demand order payment is not fully verified by the runtime.
  2. Mortality of transactions is not yet set correctly.
  3. Avoid accessing relay-chain storage where possible as this will easily break with upgrades. Most if not all accesses should be possible to avoid already (e.g. by properly using transaction mortality), where it is not yet (if there is any), adding a runtime API or similar should be considered.
  4. To make the work more accessible it should become integrated with Omni-node in some way.
  5. Handling of remaining misbehavior/use proper proofs to handle misbehaving nodes and asynchronicity. (Very much related to (1))
  6. Reward handling: While the code does determine what was spent for an order, it discards the information instead of making sure that the order placer will get compensated for the placed order accordingly. To make the system robust, the reward should consist of what was paid for the order (to ensure the order placer can not make a loss) + some actual reward on top - otherwise the parachain might stall with price fluctuations (collators refusing to order at a loss).
  7. Code audit
  8. Getting some parachain to use it, to prove its production readiness (together with the code audit).
  9. Code Quality: Quite a lot of traits for customization, which is a bit confusing to read and follow along. Consider to either remove some customizations which are unlikely to be needed (can be added on-demand) or structuring them properly in one-place, with good documentation on e.g. default behavior, that the pallet itself offers an implementation with this and that behavior, and similar. Structure the code a bit better to make it visible at a glance what is implementation detail and what is user-facing entry points.

Missing validation/yet to be done:

Does it actually work? I have not tested this yet. Anyone eager to try this out, play around with it - a short report would be valuable.

TL;DR: From review I would say that a lot of useful work has been delivered. I would believe from reading the code that it works (although it is lacking robustness in some places, as listed above). Therefore I would suggest voting yes on this payment. Any followup should need to be required to make good progress on the remaining points raised.

Edited
Reply
Up
Share
Request
3,597DOT
Status
Enactment Period20mins
Tally
100%Aye
67.2%Threshold
0%Nay
Aye21
Nay0
  • 0.0%
  • 0.0%
  • 0.0%

Threshold

Bare Aye8
Max Voters23
All votes
Call
Metadata
Timeline5
Comments

I am voting yes on this one. I agree with most of the comments @eskimor made and I consider this is valuable work heading in the right direction.

In the e2e the parachain makes progress as long as there are transactions in the pool which is the desired behaviour. This has been confirmed by an engineer on my team.

A few more comments:

  • To my understanding, the current implementation makes the assumption that on-demand is never used when the parachain uses bulk core time. I think this needs to be changed such that parachains can run with bulk core time and also use on-demand. This is very useful for example to handle bursts of traffic or to speed up some other operations like storage migrations without disrupting end-users.
  • on 4: I agree + an integration test in CI.
  • on 8: A good start would be a Westend deployment in parallel with audit.
  • on 9: +1 on less knobs is better DX

Edited

Reply
Up 2

I'm also going to aye this. I have also collected some feedback while briefly looking over the code.

  • When to buy should be moved to the runtime via a runtime api
  • So no duplicated logic for the order placement criteria is needed
  • slot_bulk_mode would get obsolete this way
  • Not want to say code quality, but some things could be rewritten. Like first checking if the local node has the keys, before checking for available cores.
Reply
Up 1