-
Notifications
You must be signed in to change notification settings - Fork 107
feat(core): revert get_account_local and get_multiple_accounts_local #368
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(core): revert get_account_local and get_multiple_accounts_local #368
Conversation
|
Nice start @vict0rcarvalh0! You did a great job finding this patch... Here some considerations |
crates/core/src/surfnet/locker.rs
Outdated
| false, | ||
| ), | ||
| Some(account) => { | ||
| if account.eq(&Account::default()) || account.lamports == 0 { |
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.
A default instance would look like:
lamports: 0
data: Vec::new()
owner: Pubkey::default() (all zeros)
executable: false
rent_epoch: 0
So checking it's redundant looking for both since looking for a Account::default(), already haves account.lamports == 0, and in the case where a account have lamports == 0 it means that the account does not exist. And after the fix of LiteSVM where accounts with 0 lamports are excluded from the accounts_db
You should be able to just
Some(account) => GetAccountResult::FoundAccount(
*pubkey, account,
false,
),Because LiteSVM now handles the deleting of accounts with 0 lamports we should just try to find it and it will returns the account or false since it does not exists.
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.
Makes sense Arthur! Just made a new commit refactoring that
crates/core/src/surfnet/locker.rs
Outdated
| false, | ||
| ), | ||
| Some(account) => { | ||
| if account.eq(&Account::default()) || account.lamports == 0 { |
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.
Same as above here :)
|
What do you think @MicaiahReid ? |
…hecks for zero-lamports accounts
MicaiahReid
left a comment
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.
Perfect, thanks @vict0rcarvalh0!
|
LGTM |
LiteSVM got merged to update set_account to remove 0 lamports accounts. So get_account_local and get_multiple_accounts_local were reverted to the previous implementation. Re-ran surfpool-core integration tests that validate reset behavior:
Both passed successfully.
LiteSVM reference: LiteSVM/litesvm#218
Fixes #332