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

feat: allow patch state to take in multiple patches + access key and account patching #124

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions examples/src/spooning.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,7 @@ async fn main() -> anyhow::Result<()> {

// Patch our testnet STATE into our local sandbox:
worker
.patch_state(
sandbox_contract.id(),
"STATE".as_bytes(),
&status_msg.try_to_vec()?,
)
.patch_state(sandbox_contract.id(), "STATE", status_msg.try_to_vec()?)
.await?;

// Now grab the state to see that it has indeed been patched:
Expand Down
5 changes: 4 additions & 1 deletion workspaces/build.rs
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");
Copy link
Member

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:

Suggested change
// 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");
near_sandbox_utils::ensure_sandbox_bin().expect("Could not install sandbox");

Copy link
Author

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?

Copy link
Member

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?

}
}
15 changes: 9 additions & 6 deletions workspaces/src/network/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,16 @@ mod server;
mod testnet;
pub(crate) mod variants;

pub(crate) use sandbox::SandboxPatchAcessKeyBuilder; //not needed directly outside of the crate
pub(crate) use sandbox::SandboxPatchStateAccountBuilder; //not needed directly outside of the crate
pub(crate) use sandbox::SandboxPatchStateBuilder; //not needed directly outside of the crate
adsick marked this conversation as resolved.
Show resolved Hide resolved
pub(crate) use variants::DEV_ACCOUNT_SEED;

pub use self::betanet::Betanet;
pub use self::info::Info;
pub use self::mainnet::Mainnet;
pub use self::sandbox::Sandbox;
pub use self::testnet::Testnet;
pub use self::variants::{
pub use betanet::Betanet;
pub use info::Info;
pub use mainnet::Mainnet;
pub use sandbox::Sandbox;
pub use testnet::Testnet;
pub use variants::{
adsick marked this conversation as resolved.
Show resolved Hide resolved
AllowDevAccountCreation, DevAccountDeployer, NetworkClient, NetworkInfo, TopLevelAccountCreator,
};
263 changes: 246 additions & 17 deletions workspaces/src/network/sandbox.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(
Copy link
Member

Choose a reason for hiding this comment

The 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 tests/patch_state.rs

Copy link
Author

Choose a reason for hiding this comment

The 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

impl Into<Vec<u8>> looks weird. Is there some precedent for such API shape? I'd just use Vec<u8> or &[u8], with a strong preference for &[u8].

Vec<u8> leaks impl details and forces needless allocation -- ideally, we don't store intermediate vecs at all and just build request string.

Copy link
Contributor

Choose a reason for hiding this comment

The 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(),
}));
Copy link
Contributor

Choose a reason for hiding this comment

The 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
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather let the caller to call .data in a loop. If we need multi-item API, I'd go for

impl std::iter::Extend<(&'_ [u8], &'_ [u8])> for SandboxPatchStateBuilder<'_> {

}

Copy link
Author

@adsick adsick May 6, 2022

Choose a reason for hiding this comment

The 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 impl std::iter::Extend<(&'_ [u8], &'_ [u8])> is better than my .data_multiple - while being a little more rust-idiomatic it does not allowing it to be called in a builder chain and looks imo worse than .data_multiple

Copy link
Contributor

Choose a reason for hiding this comment

The 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, data_multiple looks like a particualr awkward name (data itself is plural).

Maybe we want .entry and .entries here?

I'll delegate to you/dev-platform folks to make a judgement call here :)

Copy link
Member

Choose a reason for hiding this comment

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

+1 for .entry and .entries

Copy link
Author

Choose a reason for hiding this comment

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

+1 for .entry and .entries

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<()> {
Copy link
Member

Choose a reason for hiding this comment

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

rename the function to transact to be more consistent with other Transaction objects

Suggested change
pub async fn apply(self) -> anyhow::Result<()> {
pub async fn transact(self) -> anyhow::Result<()> {

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(())
}
Expand Down
Loading