-
Notifications
You must be signed in to change notification settings - Fork 808
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
base: master
Are you sure you want to change the base?
On-demand credits #5990
Conversation
There was a problem hiding this 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.
let credits = Credits::<T>::get(&sender); | ||
ensure!(spot_price <= credits, Error::<T>::InsufficientCredits); | ||
Credits::<T>::insert(&sender, credits.saturating_sub(spot_price)); | ||
}, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Precisely!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
I also thought of it, but I don't think it would be needed. |
There was a problem hiding this 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.
Co-authored-by: ordian <[email protected]>
Good idea, I'll add an e2e test. |
Looks good, thanks for taking care of this! |
Co-authored-by: Dónal Murray <[email protected]>
Co-authored-by: Dónal Murray <[email protected]>
Co-authored-by: Dónal Murray <[email protected]>
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:
The error I am getting:
Not sure if I need to set something up before running the test. |
@Szegoo assuming you're running with |
Implementation of on-demand credits as described in RFC-1