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 credits #5990

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

On-demand credits #5990

wants to merge 21 commits into from

Conversation

Szegoo
Copy link
Contributor

@Szegoo Szegoo commented Oct 9, 2024

Implementation of on-demand credits as described in RFC-1

@Szegoo Szegoo marked this pull request as ready for review October 12, 2024 09:57
Copy link
Member

@eskimor eskimor left a comment

Choose a reason for hiding this comment

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

Looks good on a first pass. I was thinking about whether we need an ED here and I think the answer is no, because the credits are non-refundable, hence you have an implicit ED based on the minimum amount an on-demand core costs. This is less than the current ED on the relay chain, but it is also not a deposit, but actual spending.

polkadot/runtime/parachains/src/on_demand/mod.rs Outdated Show resolved Hide resolved
let credits = Credits::<T>::get(&sender);
ensure!(spot_price <= credits, Error::<T>::InsufficientCredits);
Credits::<T>::insert(&sender, credits.saturating_sub(spot_price));
},
Copy link
Member

Choose a reason for hiding this comment

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

I think we might need adjustments to revenue tracking. Coretime bought with credits don't need a teleport, while coretime bought with balances does. We should also deprecate orders based on balances, to give people a heads up before later removal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so we want to make place_order_allow_death and place_order_keep_alive deprecated so we can remove them later. This makes sense.

Just to confirm that I interpreted the first part of your comment correctly:

When using credits, the DOT used to purchase them is already on the Coretime chain. However, when using rc balance to make an order, the relay chain must transfer this revenue to the Coretime chain. So, if I understand correctly, you are saying that we need to adjust revenue tracking to account for this teleport when using rc balance?

Copy link
Member

Choose a reason for hiding this comment

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

Precisely!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that the RC doesn't pay for notifying revenue, is there anything we should account for? UnpaidExecution

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so. What are you thinking about?

@Szegoo
Copy link
Contributor Author

Szegoo commented Oct 15, 2024

I was thinking about whether we need an ED here and I think the answer is no, because the credits are non-refundable, hence you have an implicit ED based on the minimum amount an on-demand core costs. This is less than the current ED on the relay chain, but it is also not a deposit, but actual spending.

I also thought of it, but I don't think it would be needed.

@Szegoo Szegoo requested a review from eskimor October 26, 2024 13:46
@seadanda seadanda self-requested a review January 27, 2025 09:25
@ggwpez ggwpez mentioned this pull request Jan 28, 2025
19 tasks
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

The implementation looks good, thank you!

I'd love to see an e2e test though, perhaps by modifying an existing zombinet-sdk test that does the purchase the old way to add a new way too.

polkadot/runtime/parachains/src/on_demand/mod.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/on_demand/mod.rs Outdated Show resolved Hide resolved
@Szegoo
Copy link
Contributor Author

Szegoo commented Jan 30, 2025

I'd love to see an e2e test though, perhaps by modifying an existing zombinet-sdk test that does the purchase the old way to add a new way too.

Good idea, I'll add an e2e test.

polkadot/runtime/parachains/src/on_demand/mod.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/on_demand/mod.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/on_demand/mod.rs Outdated Show resolved Hide resolved
@seadanda
Copy link
Contributor

seadanda commented Feb 1, 2025

Looks good, thanks for taking care of this!

@Szegoo
Copy link
Contributor Author

Szegoo commented Feb 2, 2025

I'd love to see an e2e test though, perhaps by modifying an existing zombinet-sdk test that does the purchase the old way to add a new way too.

I am attempting to run the existing E2E test before modifying it; however, I am running into an issue. I am running it as follows:

cargo test -p polkadot-zombienet-sdk-tests --features zombie-metadata -- smoke::coretime_revenue::coretime_revenue_test

The error I am getting:

---- smoke::coretime_revenue::coretime_revenue_test stdout ----
Error: Metadata error: The generated code is not compatible with the node

Caused by:
    The generated code is not compatible with the node

Not sure if I need to set something up before running the test.

@ordian
Copy link
Member

ordian commented Feb 3, 2025

@Szegoo assuming you're running with ZOMBIE_PROVIDER=native, you might need to rebuild the polkadot/polkadot-parachain/polkadot-*-worker binaries from the latest commit that are exposed in your $PATH.

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.

4 participants