Improve On_demand_assigner events #4339

bolajahmad · 2024-04-30T18:23:38Z

title: Improving on_demand_assigner emitted events

doc:

audience: Rutime User
description: OnDemandOrderPlaced event that is useful for indexers to save data related to on demand orders. Check discussion here.

Closes #4254

crates: [ 'runtime-parachain]

…ime" This reverts commit c494b17.

eskimor

Thanks! Left a few nits and triggered CI for you. Please remove unrelated formatting changes as well.

Overkillus

Hey @bolajahmad o/

Always happy to have some external contributors so if you need any help please feel free to ping me.

OnDemandOrderPlaced looking good
SpotTrafficSet needs changes

I think there is a small misunderstanding so let me clear it up:

Eskimor:

SpotTrafficSet This value will be largely useless for people, maybe we should have a SpotPriceSet?

@bolajahmad :

SpotTrafficSet is used in the update_spot_traffic call. What is the intended effect of changing the EventName to SpotPriceSet, especilly if the resulting event has the same arguments?

I assume that Eskimor meant not only renaming that event but also changing what it actually represents (to be in-line with the new name). The new event should represent the actual SpotPrice instead of the Traffic component. It seems that this might be much more useful for users.

Notice that if both SpotPriceSet as proposed by Eskimor and OnDemandOrderPlaced have the spot price then in case of an order being made we would get a double event with the same thing. This might not be undesirable IMO.

TODO:

rename SpotTrafficSet to SpotPriceSet
After updating traffic calculate spot price and deposit it in that new SpotPriceSet event

@eskimor I'm not sure if I agree with:

Issue SpotTrafficSet ... only if price changed without an order getting placed.

First of all I assume you meant SpotPriceSet otherwise it seems inconsistent with what you said earlier.

Issue here is that SpotPrice can also change when orders are placed so someone could easily follow this event thinking he has the most up to date spot price which would be false if we emit it only when price changed without an order getting placed.

I suggest always emitting this spot price update (traffic goes down due to no orders or orders boost the price up). Then we can either drop the spot price from the OnDemandOrderPlaced event or simply have it in both places for absolute clarity.

Cargo.lock

Overkillus · 2024-05-10T13:06:38Z

polkadot/runtime/parachains/src/assigner_on_demand/mod.rs

A lot of formatting nits. We use nightly fmt cargo +nightly fmt --all. Feel free to run locally and see the changes yourself.

For more details feel free to read this: https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/STYLE_GUIDE.md

Running the cargo +nightly fmt --all worked but some of the formatting remained inconsistent with the styleguide example.

I'll try to run it while paying more attention if there's an issue

polkadot/runtime/parachains/src/assigner_on_demand/mod.rs

…n module; restored changes in cargo.lock

Overkillus · 2024-05-21T11:01:57Z

Hey @bolajahmad

Thanks for fixing it! Seems we are nearly good to go and the only item missing is the PRdoc. I see that you've added it into the description of that PR but what you need to do is put it in a file in this folder: prdoc (link)

Name it pr_4339.prdoc and use the same description items as the ones you have above. I added an extra SpotPriceSet clarification and removed the link:

# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: Improving on_demand_assigner emitted events

doc:
  - audience: Runtime User
    description: |
      Registering OnDemandOrderPlaced event that is useful for indexers to save data related to on demand orders. Adds SpotPriceSet as a new event to monitor on-demand spot prices. It updates whenever the price changes due to traffic.

crates:
  - name: polkadot-runtime-parachains
    bump: minor

Once you get that done we can do a final review and merge 🚀

eskimor · 2024-05-27T13:51:44Z

bot fmt

command-bot · 2024-05-27T13:51:50Z

@eskimor https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6311702 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 4-df4e7789-5cee-4a66-870b-a2e74161ab43 to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2024-05-27T13:53:27Z

@eskimor Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6311702 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6311702/artifacts/download.

eskimor · 2024-05-28T13:00:59Z

bot fmt

command-bot · 2024-05-28T13:01:04Z

@eskimor https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6324500 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 1-20a2ab59-fbb2-4a44-86b0-9f81c975e04f to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2024-05-28T13:02:59Z

@eskimor Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6324500 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6324500/artifacts/download.

eskimor

Would be nice to get the remaining formatting inconsistencies fixed. Other than that looks good.

s0me0ne-unkn0wn · 2024-05-28T17:35:38Z

polkadot/runtime/parachains/src/assigner_on_demand/mod.rs

-		/// An order was placed at some spot price amount.
-		OnDemandOrderPlaced { para_id: ParaId, spot_price: BalanceOf<T> },
-		/// The value of the spot traffic multiplier changed.
-		SpotTrafficSet { traffic: FixedU128 },
+		/// An order was placed at some spot price amount by orderer ordered_by
+		OnDemandOrderPlaced { para_id: ParaId, spot_price: BalanceOf<T>, ordered_by: T::AccountId },
+		/// The value of the spot price has likely changed
+		SpotPriceSet { spot_price: BalanceOf<T> },


This may be problematic as you're changing the formats of the existing events here. As events from the previous block are reset after the runtime upgrade but before running any hooks, event storage must always be compatible. So, technically, this part needs more care.

Buuuut, given that, IIUC,

OnDemandOrderPlaced was never deposited by the previous version of the code, and

SpotPriceSet internal representation is the same, as BalanceOf<T> coincides with FixedU128 in all the current use cases,

it may be safe to leave it as is.

That said, some more research is needed on how safe it is.

CC @liamaharon @bkchr

* master: (93 commits) Fix broken windows build (#4636) Beefy client generic on aduthority Id (#1816) pallet-staking: Put tests behind `cfg(debug_assertions)` (#4620) Broker new price adapter (#4521) Change `XcmDryRunApi::dry_run_extrinsic` to take a call instead (#4621) Update README.md (#4623) Publish `chain-spec-builder` (#4518) Add omni bencher & chain-spec-builder bins to release (#4557) Moves runtime macro out of experimental flag (#4249) Filter workspace dependencies in the templates (#4599) parachain-inherent: Make `para_id` more prominent (#4555) Add metric to measure the time it takes to gather enough assignments (#4587) Improve On_demand_assigner events (#4339) Conditional `required` checks (#4544) [CI] Deny adding git deps (#4572) [subsytem-bench] Remove redundant banchmark_name param (#4540) Add availability-recovery from systematic chunks (#1644) Remove workspace lints from templates (#4598) `sc-chain-spec`: deprecated code removed (#4410) [subsystem-benchmarks] Add statement-distribution benchmarks (#3863) ...

title: Improving `on_demand_assigner` emitted events doc: - audience: Rutime User description: OnDemandOrderPlaced event that is useful for indexers to save data related to on demand orders. Check [discussion here](https://substrate.stackexchange.com/questions/11366/ondemandassignmentprovider-ondemandorderplaced-event-was-removed/11389#11389). Closes paritytech#4254 crates: [ 'runtime-parachain] --------- Co-authored-by: Maciej <maciej.zyszkiewicz@parity.io>

bolajahmad added 3 commits May 7, 2024 11:23

implemented logic to restore disable-runtime-api to test runtime

0f5192b

fix: removed conflicts in cargo lock

30475b3

Revert "implemented logic to restore disable-runtime-api to test runt…

33b26b0

…ime" This reverts commit c494b17.

bolajahmad force-pushed the master branch from 494c221 to 33b26b0 Compare May 7, 2024 10:27

eskimor reviewed May 10, 2024

View reviewed changes

Overkillus reviewed May 10, 2024

View reviewed changes

replaced traffic events with actual spot price, resolved formatting i…

0b9be3d

…n module; restored changes in cargo.lock

Overkillus added C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. T8-polkadot This PR/Issue is related to/affects the Polkadot network. labels May 21, 2024

bolajahmad added 2 commits May 21, 2024 14:06

doc: added prdoc file

25e5a99

doc: added prdoc file, fixed typo

bf8fc83

Merge branch 'master' into master

4b889e9

Overkillus approved these changes May 28, 2024

View reviewed changes

eskimor approved these changes May 28, 2024

View reviewed changes

eskimor added this pull request to the merge queue May 28, 2024

Merged via the queue into paritytech:master with commit 650b124 May 28, 2024
154 of 157 checks passed

s0me0ne-unkn0wn reviewed May 28, 2024

View reviewed changes

This was referenced Aug 21, 2024

Update polkadot-sdk from v1.11.0 to stable2407 moondance-labs/tanssi#659

Open

Update polkadot-sdk from v1.11.0 to stable2407 moonbeam-foundation/moonbeam#2912

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve On_demand_assigner events #4339

Improve On_demand_assigner events #4339

bolajahmad commented Apr 30, 2024 •

edited

Loading

eskimor left a comment

Overkillus left a comment •

edited

Loading

Overkillus May 10, 2024

bolajahmad May 11, 2024

Overkillus commented May 21, 2024 •

edited

Loading

eskimor commented May 27, 2024

command-bot bot commented May 27, 2024 •

edited

Loading

command-bot bot commented May 27, 2024

eskimor commented May 28, 2024

command-bot bot commented May 28, 2024 •

edited

Loading

command-bot bot commented May 28, 2024

eskimor left a comment

s0me0ne-unkn0wn May 28, 2024

Improve On_demand_assigner events #4339

Improve On_demand_assigner events #4339

Conversation

bolajahmad commented Apr 30, 2024 • edited Loading

eskimor left a comment

Choose a reason for hiding this comment

Overkillus left a comment • edited Loading

Choose a reason for hiding this comment

Overkillus May 10, 2024

Choose a reason for hiding this comment

bolajahmad May 11, 2024

Choose a reason for hiding this comment

Overkillus commented May 21, 2024 • edited Loading

eskimor commented May 27, 2024

command-bot bot commented May 27, 2024 • edited Loading

command-bot bot commented May 27, 2024

eskimor commented May 28, 2024

command-bot bot commented May 28, 2024 • edited Loading

command-bot bot commented May 28, 2024

eskimor left a comment

Choose a reason for hiding this comment

s0me0ne-unkn0wn May 28, 2024

Choose a reason for hiding this comment

bolajahmad commented Apr 30, 2024 •

edited

Loading

Overkillus left a comment •

edited

Loading

Overkillus commented May 21, 2024 •

edited

Loading

command-bot bot commented May 27, 2024 •

edited

Loading

command-bot bot commented May 28, 2024 •

edited

Loading