-
Notifications
You must be signed in to change notification settings - Fork 51
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: allow patch state to take in multiple patches + access key and account patching #124
base: main
Are you sure you want to change the base?
Changes from 11 commits
94d66ac
bda570d
74c4f4f
f351e76
0d2167d
58fa329
6a99349
991ce43
7ff6b82
2ec00fc
edceb77
50d2266
edacfd6
f8842e3
1c4c459
7e1b304
ff26068
132938f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,9 @@ | ||
fn main() { | ||
let doc_build = cfg!(doc) || std::env::var("DOCS_RS").is_ok(); | ||
if !doc_build && cfg!(feature = "install") { | ||
near_sandbox_utils::install().expect("Could not install sandbox"); | ||
// using unwrap because all the useful error messages are hidden inside | ||
near_sandbox_utils::ensure_sandbox_bin().unwrap(); | ||
// previously the next line was causing near sandbox to be installed every time cargo build/check was called | ||
// near_sandbox_utils::install().expect("Could not install sandbox"); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -4,15 +4,19 @@ use std::str::FromStr; | |||||
use async_trait::async_trait; | ||||||
use near_jsonrpc_client::methods::sandbox_fast_forward::RpcSandboxFastForwardRequest; | ||||||
use near_jsonrpc_client::methods::sandbox_patch_state::RpcSandboxPatchStateRequest; | ||||||
use near_primitives::hash::CryptoHash; | ||||||
use near_primitives::state_record::StateRecord; | ||||||
use near_primitives::types::StorageUsage; | ||||||
use near_primitives::views::AccountView; | ||||||
use std::iter::IntoIterator; | ||||||
|
||||||
use super::{AllowDevAccountCreation, NetworkClient, NetworkInfo, TopLevelAccountCreator}; | ||||||
use crate::network::server::SandboxServer; | ||||||
use crate::network::Info; | ||||||
use crate::result::CallExecution; | ||||||
use crate::rpc::client::Client; | ||||||
use crate::rpc::patch::ImportContractTransaction; | ||||||
use crate::types::{AccountId, Balance, InMemorySigner, SecretKey}; | ||||||
use crate::types::{AccountId, Balance, InMemorySigner, Nonce, SecretKey}; | ||||||
use crate::{Account, Contract, Network, Worker}; | ||||||
|
||||||
// Constant taken from nearcore crate to avoid dependency | ||||||
|
@@ -135,36 +139,261 @@ impl Sandbox { | |||||
ImportContractTransaction::new(id.to_owned(), worker.client(), self.client()) | ||||||
} | ||||||
|
||||||
pub(crate) async fn patch_state( | ||||||
pub(crate) fn patch_state(&self, account_id: AccountId) -> SandboxPatchStateBuilder { | ||||||
SandboxPatchStateBuilder::new(self, account_id) | ||||||
} | ||||||
|
||||||
pub(crate) fn patch_account(&self, account_id: AccountId) -> SandboxPatchStateAccountBuilder { | ||||||
SandboxPatchStateAccountBuilder::new(self, account_id) | ||||||
} | ||||||
|
||||||
pub(crate) fn patch_access_key( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you also include some tests for these various patches just so we can get some confidence that they're working as expected. You can probably reuse a bunch of the test code found in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we didn't tested because we waited for the feedback. Tests will be added too. |
||||||
&self, | ||||||
contract_id: &AccountId, | ||||||
key: &[u8], | ||||||
value: &[u8], | ||||||
) -> anyhow::Result<()> { | ||||||
let state = StateRecord::Data { | ||||||
account_id: contract_id.to_owned(), | ||||||
data_key: key.to_vec(), | ||||||
value: value.to_vec(), | ||||||
account_id: AccountId, | ||||||
public_key: crate::types::PublicKey, | ||||||
) -> SandboxPatchAcessKeyBuilder { | ||||||
SandboxPatchAcessKeyBuilder::new(self, account_id, public_key) | ||||||
} | ||||||
|
||||||
// shall we expose convenience patch methods here for consistent API? | ||||||
|
||||||
pub(crate) async fn fast_forward(&self, delta_height: u64) -> anyhow::Result<()> { | ||||||
// NOTE: RpcSandboxFastForwardResponse is an empty struct with no fields, so don't do anything with it: | ||||||
self.client() | ||||||
// TODO: replace this with the `query` variant when RpcSandboxFastForwardRequest impls Debug | ||||||
.query_nolog(&RpcSandboxFastForwardRequest { delta_height }) | ||||||
.await | ||||||
.map_err(|err| anyhow::anyhow!("Failed to fast forward: {:?}", err))?; | ||||||
|
||||||
Ok(()) | ||||||
} | ||||||
} | ||||||
|
||||||
//todo: review naming | ||||||
#[must_use = "don't forget to .apply() this `SandboxPatchStateBuilder`"] | ||||||
pub struct SandboxPatchStateBuilder<'s> { | ||||||
adsick marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
sandbox: &'s Sandbox, | ||||||
account_id: AccountId, | ||||||
records: Vec<StateRecord>, | ||||||
} | ||||||
|
||||||
impl<'s> SandboxPatchStateBuilder<'s> { | ||||||
pub fn new(sandbox: &'s Sandbox, account_id: AccountId) -> Self { | ||||||
SandboxPatchStateBuilder { | ||||||
sandbox, | ||||||
account_id, | ||||||
records: Vec::with_capacity(4), | ||||||
} | ||||||
} | ||||||
|
||||||
pub fn data(mut self, key: impl Into<Vec<u8>>, value: impl Into<Vec<u8>>) -> Self { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like we should have a way to delete key as well? |
||||||
let data = StateRecord::Data { | ||||||
account_id: self.account_id.clone(), | ||||||
data_key: key.into(), | ||||||
value: value.into(), | ||||||
}; | ||||||
let records = vec![state]; | ||||||
|
||||||
self.records.push(data); | ||||||
self | ||||||
} | ||||||
|
||||||
pub fn data_multiple( | ||||||
mut self, | ||||||
kvs: impl IntoIterator<Item = (impl Into<Vec<u8>>, impl Into<Vec<u8>>)>, | ||||||
) -> Self { | ||||||
let Self { | ||||||
ref mut records, | ||||||
ref account_id, | ||||||
.. | ||||||
} = self; | ||||||
records.extend(kvs.into_iter().map(|(key, value)| StateRecord::Data { | ||||||
account_id: account_id.clone(), | ||||||
data_key: key.into(), | ||||||
value: value.into(), | ||||||
})); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: there's logical duplication between the two methods, one could delegate to the other (or both could delegate to internal _state_record) |
||||||
self | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather let the caller to call impl std::iter::Extend<(&'_ [u8], &'_ [u8])> for SandboxPatchStateBuilder<'_> {
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with calling data in a loop (it does not make it ergonomic for cases where you have a vec of values tho), but I don't understand why this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point about the builder! Indeed, to look at the std, theer's
Though, in this case, Maybe we want I'll delegate to you/dev-platform folks to make a judgement call here :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
got it |
||||||
|
||||||
// pub fn access_key(mut self, public_key: &PublicKey, access_key: &AccessKey) -> Self { | ||||||
// let access_key = StateRecord::AccessKey { | ||||||
// account_id: self.account_id.clone(), | ||||||
// public_key: public_key.0.clone(), | ||||||
// access_key: access_key.clone(), | ||||||
// }; | ||||||
// self.records.push(access_key); | ||||||
// self | ||||||
// } | ||||||
adsick marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
pub async fn apply(self) -> anyhow::Result<()> { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rename the function to
Suggested change
|
||||||
let records = self.records; | ||||||
// NOTE: RpcSandboxPatchStateResponse is an empty struct with no fields, so don't do anything with it: | ||||||
let _patch_resp = self | ||||||
.sandbox | ||||||
.client() | ||||||
.query(&RpcSandboxPatchStateRequest { records }) | ||||||
.await | ||||||
.map_err(|err| anyhow::anyhow!("Failed to patch state: {:?}", err))?; | ||||||
|
||||||
Ok(()) | ||||||
} | ||||||
} | ||||||
|
||||||
pub(crate) async fn fast_forward(&self, delta_height: u64) -> anyhow::Result<()> { | ||||||
// NOTE: RpcSandboxFastForwardResponse is an empty struct with no fields, so don't do anything with it: | ||||||
self.client() | ||||||
// TODO: replace this with the `query` variant when RpcSandboxFastForwardRequest impls Debug | ||||||
.query_nolog(&RpcSandboxFastForwardRequest { delta_height }) | ||||||
#[must_use = "don't forget to .apply() this `SandboxPatchStateAccountBuilder`"] | ||||||
pub struct SandboxPatchStateAccountBuilder<'s> { | ||||||
sandbox: &'s Sandbox, | ||||||
account_id: AccountId, | ||||||
amount: Option<Balance>, | ||||||
locked: Option<Balance>, | ||||||
code_hash: Option<CryptoHash>, | ||||||
storage_usage: Option<StorageUsage>, | ||||||
} | ||||||
|
||||||
impl<'s> SandboxPatchStateAccountBuilder<'s> { | ||||||
pub const fn new(sandbox: &'s Sandbox, account_id: AccountId) -> Self { | ||||||
Self { | ||||||
sandbox, | ||||||
account_id, | ||||||
amount: None, | ||||||
locked: None, | ||||||
code_hash: None, | ||||||
storage_usage: None, | ||||||
} | ||||||
} | ||||||
|
||||||
pub const fn amount(mut self, amount: Balance) -> Self { | ||||||
self.amount = Some(amount); | ||||||
self | ||||||
} | ||||||
|
||||||
pub const fn locked(mut self, locked: Balance) -> Self { | ||||||
self.locked = Some(locked); | ||||||
self | ||||||
} | ||||||
|
||||||
pub const fn code_hash(mut self, code_hash: CryptoHash) -> Self { | ||||||
self.code_hash = Some(code_hash); | ||||||
self | ||||||
} | ||||||
|
||||||
pub const fn storage_usage(mut self, storage_usage: StorageUsage) -> Self { | ||||||
adsick marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
self.storage_usage = Some(storage_usage); | ||||||
self | ||||||
} | ||||||
|
||||||
pub async fn apply(self) -> anyhow::Result<()> { | ||||||
let account_view = self | ||||||
.sandbox | ||||||
.client() | ||||||
.view_account(self.account_id.clone(), None); | ||||||
|
||||||
let AccountView { | ||||||
amount: previous_amount, | ||||||
locked: previous_locked, | ||||||
code_hash: previous_code_hash, | ||||||
storage_usage: previous_storage_usage, | ||||||
.. | ||||||
} = account_view | ||||||
.await | ||||||
.map_err(|err| anyhow::anyhow!("Failed to fast forward: {:?}", err))?; | ||||||
.map_err(|err| anyhow::anyhow!("Failed to read account: {:?}", err))?; | ||||||
|
||||||
let account = StateRecord::Account { | ||||||
account_id: self.account_id.clone(), | ||||||
account: near_primitives::account::Account::new( | ||||||
self.amount.unwrap_or(previous_amount), | ||||||
self.locked.unwrap_or(previous_locked), | ||||||
self.code_hash.unwrap_or(previous_code_hash), | ||||||
self.storage_usage.unwrap_or(previous_storage_usage), | ||||||
), | ||||||
}; | ||||||
|
||||||
let records = vec![account]; | ||||||
|
||||||
// NOTE: RpcSandboxPatchStateResponse is an empty struct with no fields, so don't do anything with it: | ||||||
let _patch_resp = self | ||||||
.sandbox | ||||||
.client() | ||||||
.query(&RpcSandboxPatchStateRequest { records }) | ||||||
.await | ||||||
.map_err(|err| anyhow::anyhow!("Failed to patch state: {:?}", err))?; | ||||||
|
||||||
Ok(()) | ||||||
} | ||||||
} | ||||||
|
||||||
#[must_use = "don't forget to .apply() this `SandboxPatchStateAccountBuilder`"] | ||||||
pub struct SandboxPatchAcessKeyBuilder<'s> { | ||||||
sandbox: &'s Sandbox, | ||||||
account_id: AccountId, | ||||||
public_key: crate::types::PublicKey, | ||||||
nonce: Nonce, | ||||||
} | ||||||
|
||||||
impl<'s> SandboxPatchAcessKeyBuilder<'s> { | ||||||
pub const fn new( | ||||||
sandbox: &'s Sandbox, | ||||||
account_id: AccountId, | ||||||
public_key: crate::types::PublicKey, | ||||||
) -> Self { | ||||||
Self { | ||||||
sandbox, | ||||||
account_id, | ||||||
public_key, | ||||||
nonce: 0, | ||||||
} | ||||||
} | ||||||
|
||||||
pub const fn nonce(mut self, nonce: Nonce) -> Self { | ||||||
self.nonce = nonce; | ||||||
self | ||||||
} | ||||||
|
||||||
pub async fn full_access(self) -> anyhow::Result<()> { | ||||||
let mut access_key = near_primitives::account::AccessKey::full_access(); | ||||||
access_key.nonce = self.nonce; | ||||||
let access_key = StateRecord::AccessKey { | ||||||
account_id: self.account_id, | ||||||
public_key: self.public_key.into(), | ||||||
access_key, | ||||||
}; | ||||||
|
||||||
let records = vec![access_key]; | ||||||
|
||||||
// NOTE: RpcSandboxPatchStateResponse is an empty struct with no fields, so don't do anything with it: | ||||||
let _patch_resp = self | ||||||
.sandbox | ||||||
.client() | ||||||
.query(&RpcSandboxPatchStateRequest { records }) | ||||||
.await | ||||||
.map_err(|err| anyhow::anyhow!("Failed to patch state: {:?}", err))?; | ||||||
|
||||||
Ok(()) | ||||||
} | ||||||
|
||||||
pub async fn function_call_access( | ||||||
self, | ||||||
receiver_id: &AccountId, | ||||||
method_names: &[&str], | ||||||
allowance: Option<Balance>, | ||||||
) -> anyhow::Result<()> { | ||||||
let mut access_key: near_primitives::account::AccessKey = | ||||||
crate::types::AccessKey::function_call_access(receiver_id, method_names, allowance) | ||||||
.into(); | ||||||
access_key.nonce = self.nonce; | ||||||
let access_key = StateRecord::AccessKey { | ||||||
account_id: self.account_id, | ||||||
public_key: self.public_key.into(), | ||||||
access_key, | ||||||
}; | ||||||
|
||||||
let records = vec![access_key]; | ||||||
|
||||||
// NOTE: RpcSandboxPatchStateResponse is an empty struct with no fields, so don't do anything with it: | ||||||
let _patch_resp = self | ||||||
.sandbox | ||||||
.client() | ||||||
.query(&RpcSandboxPatchStateRequest { records }) | ||||||
.await | ||||||
.map_err(|err| anyhow::anyhow!("Failed to patch state: {:?}", err))?; | ||||||
|
||||||
Ok(()) | ||||||
} | ||||||
|
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.
nice, but can you remove the comments surrounding it and also change the unwrap to an expect, since the error messages can be confusing at times when they don't give context:
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.
Yes, I agree about the context part, but there also should be a note of 'precise' reason of failure.
I'll try to do something like "Could not install sandbox. Reason: ... "
That sounds good?
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.
So after #134 we won't be able to change it to
ensure_sandbox_bin()
, but to alleviate this issue with re-installing, we'll also resolve it along with #88. It just requires a more involved versioning mechanism. Sounds good to you?