-
Notifications
You must be signed in to change notification settings - Fork 654
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
feat(runtime): switch to state staking #2272
Conversation
Addresses spec change near/NEPs#41 Test plan --------- All runtime tests pass (removing ones that don't make sense) Tests that remove account when it's under funded still work as expected.
Let's wait with the implementation until we agree with the design here: near/NEPs#40 . |
@nearmax Generally, I think spec changes should come with associated code changes after initial discussion - because if the change is dramatic or has huge risk, it's valuable to see that at the spec discussion state. It also clear after even initial implementation what are edge cases that should be added to the spec. |
Okay, do you want to submit spec changes PR or create an issue? |
@nearmax Submitted spec change. |
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.
Except for the obviously broken migration, LGTM.
storage_paid_at
and rent_paid
getting default 0 values for RPC responses should not break AppLayer, in my opinion, so we can deprecate the fields when we have a plan of how we do that (tracking issue: #2271)
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.
Also, it's possible that we might need to bump balance on all accounts that have code deployed. Otherwise they might be out of balance in the testnet. But it's probably not as bad, because this state is not enforced actually. Which we should do
@evgenykuzyakov Returned back to 100 and made sure in migration that every account has enough balance to cover storage. |
RentUnpaid { | ||
/// An account which is required to pay the rent | ||
/// Signer account doesn't have enough balance after transaction. | ||
LackBalanceForState { |
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.
InsufficientBalanceForState
would be a better name.
/// - Err(StorageError::StorageInconsistentState) if account has invalid storage usage or amount/locked. | ||
/// | ||
/// Read details of state staking https://nomicon.io/Economics/README.html#state-stake | ||
pub(crate) fn get_insufficient_storage_stake( |
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 would rename it to check_sufficient_storage_stake
.
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.
Approving, assuming the comments are addressed.
Addresses spec change near/NEPs#41
Associated PR to change nearlib errors - near/near-api-js#270
Test plan
All runtime tests pass (removing ones that don't make sense)
Tests that remove account when it's under funded still work as expected.
TODO: add test for creation w/o funds + transfer and deletion of under funded account.