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

Coretime auto renew #4351

Closed
bkchr opened this issue May 2, 2024 · 7 comments · Fixed by #4424
Closed

Coretime auto renew #4351

bkchr opened this issue May 2, 2024 · 7 comments · Fixed by #4424
Labels
I5-enhancement An additional feature request.

Comments

@bkchr
Copy link
Member

bkchr commented May 2, 2024

Currently parachains that want to renew their slot need to do this every sale in the first week. They can also do this afterwards, but then all cores could already have been sold. It would be nice to support an auto renew. Parachains could opt-in to this auto renew and then only would need to ensure that the account for doing the auto renew has enough funds.

Basically the following transaction should be added:

fn enable_auto_renew(core, target_account)

First time this is called, the core should be renewed automatically (maybe it was already and we need to check "the future") and then the core & account is to some list. When rotating the sales this list should be iterated and all cores should be automatically renewed.

@bkchr
Copy link
Member Author

bkchr commented May 2, 2024

CC @seadanda

@bkchr bkchr added the I5-enhancement An additional feature request. label May 2, 2024
@itsbirdo
Copy link
Member

itsbirdo commented May 2, 2024

Would be a welcome improvement based on feedback from several teams

@Szegoo
Copy link
Contributor

Szegoo commented May 3, 2024

Would like to work on this over the weekend if available.

@Szegoo
Copy link
Contributor

Szegoo commented May 3, 2024

fn enable_auto_renew(core, target_account)

This should be a two step process, otherwise a parachain could set any account as target without requiring the target_account's approval for this. Another option would be, assuming this is called with the parachain's sovereing account is to set the target to the sovereign account.

@bkchr did you have something else in mind maybe?

@bkchr
Copy link
Member Author

bkchr commented May 4, 2024

@bkchr did you have something else in mind maybe?

No. Just didn't thought enough :D Probably the best is to not have target_account at all. Just take the account that signed the transaction as payment account.

@Szegoo
Copy link
Contributor

Szegoo commented May 4, 2024

Just take the account that signed the transaction as payment account.

But this can be just any account? For example, I could set myself as the payment account, drain my account, and then upon the next renewal, it would fail.

Given that the broker pallet is quite abstract, it doesn't assume that cores are assigned to parachains. But would it be acceptable to assume each 'task' has an account associated with it? (basically each parachain has its sovereign account)

I would probably add a new type to the broker pallet config which would implement conversion from TaskId to AccountId. We could reuse the LocationToAccountId implementation such that we convert Parachain(TaskId)(practically getting the parachain's sovereign account). And then this account would be used for payment.

@bkchr
Copy link
Member Author

bkchr commented May 8, 2024

But this can be just any account? For example, I could set myself as the payment account, drain my account, and then upon the next renewal, it would fail.

Yes that is right 🙈

would probably add a new type to the broker pallet config which would implement conversion from TaskId to AccountId. We could reuse the LocationToAccountId implementation such that we convert Parachain(TaskId)(practically getting the parachain's sovereign account). And then this account would be used for payment.

Sounds like a reasonable approach and probably the best we can do.

@Szegoo Szegoo mentioned this issue May 9, 2024
1 task
github-merge-queue bot pushed a commit that referenced this issue Aug 5, 2024
This PR adds functionality that allows tasks to enable auto-renewal.
Each task eligible for renewal can enable auto-renewal.

A new storage value is added to track all the cores with auto-renewal
enabled and the associated task running on the core. The `BoundedVec` is
sorted by `CoreIndex` to make disabling auto-renewal more efficient.

Cores are renewed at the start of a new bulk sale. If auto-renewal
fails(e.g. due to the sovereign account of the task not holding
sufficient balance), an event will be emitted, and the renewal will
continue for the other cores.

The two added extrinsics are:
- `enable_auto_renew`: Extrinsic for enabling auto renewal.
- `disable_auto_renew`: Extrinsic for disabling auto renewal.

TODOs:
- [x] Write benchmarks for the newly added extrinsics.

Closes: #4351

---------

Co-authored-by: Dónal Murray <donalm@seadanda.dev>
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this issue Aug 28, 2024
This PR adds functionality that allows tasks to enable auto-renewal.
Each task eligible for renewal can enable auto-renewal.

A new storage value is added to track all the cores with auto-renewal
enabled and the associated task running on the core. The `BoundedVec` is
sorted by `CoreIndex` to make disabling auto-renewal more efficient.

Cores are renewed at the start of a new bulk sale. If auto-renewal
fails(e.g. due to the sovereign account of the task not holding
sufficient balance), an event will be emitted, and the renewal will
continue for the other cores.

The two added extrinsics are:
- `enable_auto_renew`: Extrinsic for enabling auto renewal.
- `disable_auto_renew`: Extrinsic for disabling auto renewal.

TODOs:
- [x] Write benchmarks for the newly added extrinsics.

Closes: paritytech#4351

---------

Co-authored-by: Dónal Murray <donalm@seadanda.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants