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

on-demand: Improve/fix events #4254

Closed
eskimor opened this issue Apr 23, 2024 · 9 comments · Fixed by #4339
Closed

on-demand: Improve/fix events #4254

eskimor opened this issue Apr 23, 2024 · 9 comments · Fixed by #4339
Assignees

Comments

@eskimor
Copy link
Member

eskimor commented Apr 23, 2024

I just checked on events in on-demand and noticed two things:

  1. OnDemandOrderPlaced seems to be never triggered.
  2. SpotTrafficSet This value will be largely useless for people, maybe we should have a SpotPriceSet?
@eskimor eskimor converted this from a draft issue Apr 23, 2024
@bolajahmad
Copy link
Contributor

Hi, I would like to take this on. I wonder what was the intended purpose for the OnDemandOrderPlaced?

  1. Should this not be added in the extrinsics like do_place_order and/or on_demand_order? I can work it in there instead of removing it altogether.
  2. 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?

@eskimor
Copy link
Member Author

eskimor commented Apr 29, 2024

Relevant stackexchange question: https://substrate.stackexchange.com/questions/11366/ondemandassignmentprovider-ondemandorderplaced-event-was-removed/11389#11389

What is the intended effect of changing the EventName to SpotPriceSet,

That we are not reporting the abstract spot traffic, but the actual current price. (spot traffic multiplied by base fee)

Hi, I would like to take this on.

Amazing! Thanks a lot! Happy to review!

Should this not be added in the extrinsics like do_place_order and/or on_demand_order? I can work it in there instead of removing it altogether.

I don't think we should remove it at all.

I guess we should do the following:

  1. Issue OnDemandOrderPlaced whenever an order is placed - giving you the current price.
  2. Issue SpotTrafficSet ... only if price changed without an order getting placed.

@cuckooemm
Copy link

OnDemandOrderPlaced it is better to add an ordering account

@cuckooemm
Copy link

If the traffic value is only used to calculate the real-time price, it is better to display the price directly. If it has other functions, just keep it like this.

@eskimor
Copy link
Member Author

eskimor commented Apr 30, 2024

Thanks for the feedback! :-)

@eskimor
Copy link
Member Author

eskimor commented Apr 30, 2024

@bolajahmad if you still want to take this, do you have some timeline in mind?

@bolajahmad
Copy link
Contributor

@eskimor, I have been following the responses in my mail, and I am able to respond now.

I will start working on this and in a couple hours (approx 36) I will have a PR for review.

@bolajahmad
Copy link
Contributor

OnDemandOrderPlaced it is better to add an ordering account

I don't get what you mean. The OnDemandOrderPlaced only expects para_id and spot_price. Are you just saying a 3rd property should be added which is the account of the orderer?

@cuckooemm
Copy link

OnDemandOrderPlaced it is better to add an ordering account

I don't get what you mean. The OnDemandOrderPlaced only expects para_id and spot_price. Are you just saying a 3rd property should be added which is the account of the orderer?

yes

@eskimor eskimor moved this from Backlog to In Progress in parachains team board May 6, 2024
github-merge-queue bot pushed a commit that referenced this issue May 28, 2024
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 #4254 

crates: [ 'runtime-parachain]

---------

Co-authored-by: Maciej <[email protected]>
@github-project-automation github-project-automation bot moved this from In Progress to Completed in parachains team board May 28, 2024
girazoki pushed a commit to moondance-labs/polkadot-sdk that referenced this issue May 30, 2024
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 <[email protected]>
hitchhooker pushed a commit to ibp-network/polkadot-sdk that referenced this issue Jun 5, 2024
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 <[email protected]>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this issue Aug 2, 2024
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed
Development

Successfully merging a pull request may close this issue.

4 participants