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

Adjust to new Broker pallet. #334

Merged

Conversation

eskimor
Copy link
Contributor

@eskimor eskimor commented May 29, 2024

  • Does not require a CHANGELOG entry

@eskimor eskimor mentioned this pull request May 29, 2024
7 tasks
@muharem
Copy link
Contributor

muharem commented May 30, 2024

@eskimor is it still draft or ready for review? also some required CIs are falling

@eskimor
Copy link
Contributor Author

eskimor commented Jun 3, 2024

@muharem so far draft. The polkadot-sdk release this PR relies on does not exist yet.

EgorPopelyaev pushed a commit to paritytech/polkadot-sdk that referenced this pull request Jun 4, 2024
Related to #4521 and
#4656

This allows a release of the Broker pallet so the new price adapter can
be tested on Kusama ASAP
(polkadot-fellows/runtimes#334).
@eskimor eskimor marked this pull request as ready for review June 4, 2024 16:03
Copy link
Contributor

@seadanda seadanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it just needs an update to one pricing-related failure in a test. Otherwise LGTM

CHANGELOG.md Outdated Show resolved Hide resolved
@eskimor eskimor changed the title WIP: Adjust to new Broker pallet. Adjust to new Broker pallet. Jun 5, 2024
Co-authored-by: Dónal Murray <donalm@seadanda.dev>
@eskimor
Copy link
Contributor Author

eskimor commented Jun 5, 2024

Test is fixed. For adjusting the base price, I was thinking about writing a migration, but we could equally well let the price controller do its thing: It should adjust downwards itself in the next cycle. Only "downside" would be that the minimum price stays the same for another cycle.

@seadanda
Copy link
Contributor

seadanda commented Jun 5, 2024

If this enacts before rotate_sale and at least one core is sold on the open market we start at 230 KSM, decreasing with a step to a minimum of 2.3.

If no cores are sold it will be 2300 down to 23 and we'll lose a month of info about the new pricing model. Currently we're in the fixed price stage and sellout price is still None, so that's quite likely IMO.

If it enacts after rotate_sale then it will be adapted by the old price adapter relative to the number of cores sold. currently we've sold 7/15 (renewals) and we're at the fixed price, so the base price would be about 12.8 KSM. That means the price varies between 1280 and 12.8. But in this case the new renewals coming off a lease will have a high price IIUC.

After weighing that up I would say a migration for the base price makes sense, but only if we are sure we can enact and perform the runtime upgrade in time before the sale is rotated, otherwise I would say we schedule it for the interlude and add a migration for the renewal prices for those coming off their leases the next month who have high prices since 12 KSM isn't an awful minimum. WDYT?

@eskimor
Copy link
Contributor Author

eskimor commented Jun 5, 2024

If this enacts before rotate_sale and at least one core is sold on the open market we start at 230 KSM, decreasing with a step to a minimum of 2.3.

Yep, this scenario would be fine.

But the same would be true if no core would be sold as we then take the current base price as target price.

That means the price varies between 1280 and 12.8. But in this case the new renewals coming off a lease will have a high price IIUC.

Indeed the renewal price would then be 128 KSM, which is a bit much. They could go down and wait for the market, but then losing their renewal guarantee.

So yeah, a migration adjusting the renewal price to the current price makes sense. We should also adjust periods - the fix price length can be much shorter.

@seadanda
Copy link
Contributor

seadanda commented Jun 5, 2024

But the same would be true if no core would be sold as we then take the current base price as target price.

Ah great, knew I was missing something - this makes more sense. I was mixing up the target price and base/min/regular/fixed price (🫠).

So yeah, a migration adjusting the renewal price to the current price makes sense. We should also adjust periods - the fix price length can be much shorter.

I agree. However in general this is the minimum time that the secondary markets have to operate within, so we don't want to go too short on it. Maybe roughly 1:1 primary:secondary? Or what were you thinking? I suppose it could even be 2:1 if we expect the target price to be usually correct long term.

@eskimor
Copy link
Contributor Author

eskimor commented Jun 5, 2024

Good point on secondary markets! I was thinking 2:1. 2 weeks leadin, 1 week fixed, but we can also leave it as is, because price wise we are actually fine:

If the migration hits before rotate sale - we already concluded that we are fine, if it hits after then the price for renewals is already determined by the old controller - so we are good as well. On the next sale the prices will be adjusted already.

So lease renewal price will be 12.8 not 128.

@eskimor
Copy link
Contributor Author

eskimor commented Jun 5, 2024

@muharem ready for review.

Comment on lines -256 to -259
#[cfg(feature = "runtime-benchmarks")]
type PriceAdapter = pallet_broker::Linear;
#[cfg(not(feature = "runtime-benchmarks"))]
type PriceAdapter = PriceAdapter;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for my education: are benchmarks updated in a separate PR? what is the current process? #324?

@joepetrowski
Copy link
Contributor

/merge

@fellowship-merge-bot fellowship-merge-bot bot merged commit 2539cb5 into polkadot-fellows:main Jun 5, 2024
42 checks passed
@fellowship-merge-bot
Copy link
Contributor

Enabled auto-merge in Pull Request

Available commands
  • /merge: Enables auto-merge for Pull Request
  • /merge cancel: Cancels auto-merge for Pull Request
  • /merge help: Shows this menu

For more information see the documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants