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 9 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?

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

pub(crate) use sandbox::SandboxPatchStateAccountBuilder; //not needed directly outside of the crate
pub(crate) use sandbox::SandboxPatchStateBuilder; //not needed directly outside of the crate
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,
};
179 changes: 161 additions & 18 deletions workspaces/src/network/sandbox.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,20 @@ 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::account::AccessKey;
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, PublicKey, SecretKey};
use crate::{Account, Contract, Network, Worker};

// Constant taken from nearcore crate to avoid dependency
Expand Down Expand Up @@ -135,36 +140,174 @@ impl Sandbox {
ImportContractTransaction::new(id.to_owned(), worker.client(), self.client())
}

pub(crate) async fn patch_state(
&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(),
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)
}

// 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
}

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(())
}
}

#[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(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 })
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(())
}
Expand Down
58 changes: 53 additions & 5 deletions workspaces/src/worker/impls.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
use crate::network::{AllowDevAccountCreation, NetworkClient, NetworkInfo, TopLevelAccountCreator};
use crate::network::{Info, Sandbox};
use crate::network::{Info, Sandbox, SandboxPatchStateAccountBuilder, SandboxPatchStateBuilder};
use crate::result::{CallExecution, CallExecutionDetails, ViewResultDetails};
use crate::rpc::client::{Client, DEFAULT_CALL_DEPOSIT, DEFAULT_CALL_FN_GAS};
use crate::rpc::patch::ImportContractTransaction;
use crate::types::{AccountId, Gas, InMemorySigner, SecretKey};
use crate::types::{AccountId, Gas, InMemorySigner, PublicKey, SecretKey};
use crate::worker::Worker;
use crate::{Account, Block, Contract};
use crate::{AccountDetails, Network};
use async_trait::async_trait;
use near_primitives::account::AccessKey;
use near_primitives::types::Balance;
use std::collections::HashMap;

Expand Down Expand Up @@ -174,16 +175,63 @@ impl Worker<Sandbox> {
self.workspace.import_contract(id, worker)
}

/// Patch state into the sandbox network, using builder pattern. This will allow us to set
/// state that we have acquired in some manner. This allows us to test random cases that
/// are hard to come up naturally as state evolves.
pub fn patch_state_builder(&self, account_id: &AccountId) -> SandboxPatchStateBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

Think it's better just to have one patch_state function that way we can be consistent and have a defacto way of doing patching state. Let's merge this one and the other and just have this builder like one be the default

Copy link
Author

Choose a reason for hiding this comment

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

Let's merge this one and the other

which 'other' one?

Copy link
Member

Choose a reason for hiding this comment

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

I meant we can merge both patch_state and patch_state_builder to be just patch_state(AccountId) -> Builder. Would be a breaking API change, but it's fine since it would align with the other builder functions, and better to be consistent that way

self.workspace.patch_state(account_id.clone())
}

/// Patch account state using builder pattern
pub fn patch_account(&self, account_id: &AccountId) -> SandboxPatchStateAccountBuilder {
self.workspace.patch_account(account_id.clone())
}

/// Patch state into the sandbox network, given a key and value. This will allow us to set
/// state that we have acquired in some manner. This allows us to test random cases that
/// are hard to come up naturally as state evolves.
pub async fn patch_state(
&self,
contract_id: &AccountId,
key: &[u8],
value: &[u8],
key: impl Into<Vec<u8>>,
value: impl Into<Vec<u8>>,
Copy link
Member

Choose a reason for hiding this comment

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

not everything will impl an Into<Vec<u8>> and by that notion won’t fit all cases. Best to keep as-is to avoid that situation, since in rust a lot of things can be converted into a series of bytes &[u8] including Vec<u8>. This will also avoid having to make a breaking change for this since now people would have to require this trait impl.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah these traits are not as intuitive for a developer to read as well as it sticks us with using this API even when we remove the requirement for bytes to be converted to Vec<u8>

) -> anyhow::Result<()> {
self.workspace
.patch_state(contract_id.clone())
.data(key, value)
.apply()
.await?;
Ok(())
}

//todo: add more patch state methods like the previous one

/// Patch state into the sandbox network. Same as `patch_state` but accepts a sequence of key value pairs
pub async fn patch_state_multiple(
&self,
account_id: &AccountId,
kvs: impl IntoIterator<Item = (impl Into<Vec<u8>>, impl Into<Vec<u8>>)>,
) -> anyhow::Result<()> {
self.workspace
.patch_state(account_id.clone())
.data_multiple(kvs)
.apply()
.await?;
Ok(())
}

pub async fn patch_access_key(
&self,
account_id: &AccountId,
public_key: &PublicKey,
access_key: &AccessKey,
) -> anyhow::Result<()> {
self.workspace.patch_state(contract_id, key, value).await
self.workspace
.patch_state(account_id.clone())
.access_key(public_key, access_key)
.apply()
.await?;
Ok(())
}

/// Fast forward to a point in the future. The delta block height is supplied to tell the
Expand Down
Loading