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

chore: update to 0.13.0 nearcore crates #820

Merged
merged 7 commits into from
May 21, 2022
Merged

Conversation

austinabell
Copy link
Contributor

WIP since the crates haven't been released, opening PR for visibility and for input as a few things had to change:

  • Receipt structure removed receipt_indices, and added non_exhaustive
    • Not sure why the indices would be needed, this struct was added in 4.0 pre-releases so it's fine to remove
  • Remove outcome function on the MockedBlockchain
    • This was misleading as it only captured some gas costs, and with the new protocol change and updates, this API was not possible because getting the outcome required taking ownership of VMLogic struct
  • Slightly updated interface for created_receipts and logs (nothing too breaking)
  • Some additional conversions to be able to format Receipts in a usable way (like serializing Pubkey to string and back)

Most importantly, this change comes with the functionality of having the correct promise_batch_action_function_call_weight implementation by using the new protocol feature

near-bulldozer bot pushed a commit to near/nearcore that referenced this pull request May 19, 2022
Needed to retain functionality for unit testing within the SDK

near/near-sdk-rs#820 is the matching PR to the SDK (and other changes needed when updating for reference)
Copy link
Member

@ChaoticTempest ChaoticTempest left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +179 to +182
fn pub_key_conversion(key: &VmPublicKey) -> PublicKey {
// Hack by serializing and deserializing the key. This format should be consistent.
String::from(key).parse().unwrap()
}
Copy link
Member

Choose a reason for hiding this comment

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

feels like this conversion could also fall into the trap of semantics changing from under us if near_crypto::PublicKey string conversion ever changes, so a test might be worthy. This isn't too important though since this is only gonna be in unit tests, so leaving this one up to you if you wanna add

#[allow(dead_code)]
#[cfg(test)]
pub(crate) static mut NEXT_TRIE_OBJECT_INDEX: u64 = 0;
/// Get next id of the object stored on trie.
#[allow(dead_code)]
#[cfg(test)]
Copy link
Member

Choose a reason for hiding this comment

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

hmm, since this is the test_utils module don't we want to just expand further and say the whole module is under #[cfg(test)] instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, because these are annotated with this because they are just used within the crate, where the others are used because they are used externally (you can't use things marked with cfg(test) externally

Copy link
Contributor

@itegulov itegulov left a comment

Choose a reason for hiding this comment

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

Seems very reasonable to me

Comment on lines +170 to +175
PrimitivesAction::DeleteKey(k) => {
VmAction::DeleteKey { public_key: pub_key_conversion(&k.public_key) }
}
PrimitivesAction::DeleteAccount(a) => {
VmAction::DeleteAccount { beneficiary_id: a.beneficiary_id.parse().unwrap() }
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Very small nit: any reason why you didn't inline these like you have done with others above? Sorry for nitpicking, this is just triggering my OCD :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's just default formatting, the line is too long and it pushes it like this

@austinabell austinabell merged commit d711a0f into master May 21, 2022
@austinabell austinabell deleted the austin/nc_13_update branch May 21, 2022 19:20
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.

3 participants