feat: convenience functions for `Contract` and `Account` #33

ChaoticTempest · 2021-12-08T17:54:26Z

requires #25 to be merged in first.

addresses one of the bullet points in #28

Account/Contract Convenience Functions

let worker = workspaces::sandbox();
let contract = worker.dev_deploy(...).await?;
let account = worker.dev_create().await?;

account.call(&worker, contract_id, "function".into())
   .with_args( ... )
   .with_gas( /*optional: defaults to an internal constant */ )
   .with_deposit( /* optional: defaults to 0 */)
   .transact()
   .await?;

Can call directly into the contract using the contract's signer to sign the transaction itself. Does not require to specify the contract_id

contract.call(&worker, "function".into())
   .with_args( ... )
   .with_gas( /*optional: defaults to an internal constant */ )
   .with_deposit( /* optional: defaults to 0 */)
   .transact()
   .await?;

account.transfer_near, account.delete_account, account.deploy and contract.delete_contract now available

…uilder

workspaces/src/network/account.rs

TrevorJTClarke · 2021-12-13T21:19:23Z

More of a context question:

    let agent = worker.dev_create().await?;
    println!("AGENT: {}", agent.id());

What would the method be for mainnet support? worker.create?
I would expect I should be able to do this, given i have an account that has balance and can create sub-accounts.

willemneal · 2021-12-13T21:32:45Z

@ChaoticTempest shouldn't it be account_id to stay consistent with the sdk?

ChaoticTempest · 2021-12-13T22:51:02Z

@TrevorJTClarke Equivalent in mainnet would be worker.create_tla/worker.create_tla_and_deploy. I haven't exposed sub-account creation yet, but let me add it to this PR too since it's another convenience function for account.

Creating a sub account should look like this:

let subaccount = account.create_account("subaccount")
    .with_initial_balance(parse_near!("1 N")); // optional: defaults to 0.1 Near. Enough to hold storage
    .with_key(secret_key)  // optional: if not specified, generate it
    .transact()
    .await?;

println!("account id: {:?}", account.id());  // should yield "dev-{NUM}"
println!("subaccount id: {:?}", subaccount.id());   // should yield "subaccount.dev-{NUM}"

@willemneal

@ChaoticTempest shouldn't it be account_id to stay consistent with the sdk?

You mean how we reference .id() instead of .account_id()? I think that's only apart of near-sdk-sim so we don't need to make it consistent just quite.

workspaces/src/network/account.rs

austinabell · 2021-12-14T02:25:13Z

workspaces/src/network/account.rs

+        parent_id: AccountId,
+        new_account_id: String,


I'm a little confused on why we are forcing the account to be built as a subaccount. If this is specifically intended for that flow, perhaps we should rename to indicate this?

I'll just rename it to be create_subaccount then. This was initially to be consistent with workspaces-js/Account, but better to be explicit here

is the fact that it's a subaccount obfuscated completely from the API? The only issue I see is just that it may be doing something differently than a developer may expect, which is a bit of a foot gun. Maybe we should consider renaming on js side for consistency as well if we want to make this change?

is the fact that it's a subaccount obfuscated completely from the API?

Yup, it's pretty much that. When we go to call into account creation, there's no flag to say, "hey let's go make a subaccount". Instead, it's just based on account id we give it such as "A" or "B.A"

Maybe we should consider renaming on js side for consistency as well if we want to make this change?

The js side is pretty flexible (maybe too flexible). The create_account on that side allows you to create both top level and subaccounts regardless of what account context is present. Changing it on the js side would probably require a good amount of rework. On rust side, I've just had it be explicit in the functionality. If you want to create a TLA account you call worker.create_tla, and subaccounts with account.create_subaccount

okay, I think I might have to look through code when I get a chance to understand this difference, thanks for giving context!

workspaces/src/rpc/client.rs

workspaces/src/network/account.rs

matklad

LGTM overall!

There's some awkwardness in API here and there, which is totally fine for the current state of iteration!

examples/src/spooning.rs

workspaces/src/network/account.rs

matklad · 2021-12-15T10:54:50Z

workspaces/src/network/account.rs

+        &self,
+        worker: &Worker<T>,
+        receiver_id: AccountId,
+        amount_yocto: Balance,


This can be named better. _yocto perfix seems like a smell (0.9 confidence), and Ballance feels like a weirdly non-specific name (0.7 confidence, not familiar with domain). I'd expect amount: Near.

Balance is the type that gets used everywhere for this even such as api-js. It make sense for it to be balance since you are transferring Balances between accounts. The name Near is already a bit bloated in the ecosystem is the thing. How about I just have it be amount: Balance? - since Balance is by default yocto and the expectation would be that a user use one of the utility functions such as pase_near!("10 N")

workspaces/src/network/account.rs

matklad · 2021-12-15T10:57:04Z

workspaces/src/network/account.rs

@@ -40,7 +101,191 @@ impl Contract {
        &self.account.id
    }

+    pub fn as_account(&self) -> &Account {


Maybe fn owner(&self) -> &Account

The contract's account is not necessarily the owner though, since the behavior for an owner can belong to the parent account or the account that created this Contract/Account

workspaces/src/network/result.rs

matklad · 2021-12-15T11:04:37Z

workspaces/tests/create_account.rs

+#[tokio::test]
+async fn test_subaccount_creation() -> anyhow::Result<()> {
+    let worker = workspaces::sandbox();
+    let account = worker.dev_create().await?;


dev_create seems like less than ideal name. Dev create what? Maybe worker.test_account() or worker.new_test_account()? worker.test_account() which always returns the same account sounds like a potentially good idea.

so I wanted to tie this with dev_deploy since they're very similar except dev_deploy also deploys a contract. How about dev_create_account()? Returning the same account will be problematic for testnet since that could cause some non-determinism if every testnet test used the same account and modified the accounts state

Returning the same account will be problematic for testnet since that could cause some non-determinism if every testnet test used the same account

I mean that "the same" is scoped to the worker -- basically, "create a new dev account" is something you use rarely, and for most tests, which need just a single account, you use "get the test account", which is created on the first access.

Not sure whether this is a good idea...

Thinking more about this, it's probably a good idea -- for the first release we should aim for orthogonal API, and we can add shortcuts later.

Potentially horrible idea this is async function, so the user already has to type .await. So what we can do is

let account_without_code = worker.dev_create_account().await?; let account_with_code = worker.dev_create_account().with_code(&wasm).await?;

This reads pretty nice at the call-site, but would be quite mind bending in the rustdoc. I wonder if there's some precedent somewhere for such async builders?

I mean that "the same" is scoped to the worker -- basically, "create a new dev account" is something you use rarely, and for most tests, which need just a single account, you use "get the test account", which is created on the first access.

That pretty much is the root_account for Sandbox which covers most of those cases. For Testnet we can emulate this by doing what you describe.

Thinking more about this, it's probably a good idea -- for the first release we should aim for orthogonal API, and we can add shortcuts later.

Wait, did you mean bad idea instead of good idea here based on you saying we should keep it separate or orthogonal.

I wonder if there's some precedent somewhere for such async builders?

hmm, yeah's let consider this for the release after the first one I'm making today or tomorrow. Would be interesting if we made the builder awaitable as that could replace the transact() calls

matklad · 2021-12-15T11:07:10Z

workspaces/src/network/account.rs

+        self
+    }
+
+    pub async fn transact(self) -> anyhow::Result<CallExecutionDetails> {


Somewhat awkward name, not sure what would be better .call perhaps?

I think the rationale here, and just my assumption, is just that transact makes it more clear that it's signing and sending a transaction rather than just calling a function in a view context, but this might be intuitive for a developer anyway.

workspaces/src/rpc/client.rs

workspaces/src/network/account.rs

austinabell · 2021-12-15T15:13:26Z

workspaces/src/network/account.rs

+        self
+    }
+
+    pub async fn transact(self) -> anyhow::Result<CallExecutionDetails> {


I think the rationale here, and just my assumption, is just that transact makes it more clear that it's signing and sending a transaction rather than just calling a function in a view context, but this might be intuitive for a developer anyway.

workspaces/src/network/result.rs

austinabell

One thing I will note is that this is missing a lot of docs, we can always make a pass through before releasing to make sure it's all there, but might be better to do some now when you have the context

workspaces/src/network/account.rs

TrevorJTClarke · 2021-12-16T01:00:30Z

workspaces/src/network/account.rs

@@ -79,6 +79,22 @@ impl Account {
            new_account_id,
        )
    }
+
+    pub async fn deploy<T: Network>(


@ChaoticTempest HOW IN THE WORLD ARE YOU THIS FAST

¯\_(ツ)_/¯

austinabell

LGTM, definitely an improvement. The only note I'd make is that the examples it might be more clear if the account signing the transactions are an account other than the contract itself, since this is an uncommon pattern in practice

workspaces/src/network/result.rs

austinabell · 2021-12-16T01:18:31Z

workspaces/src/network/account.rs

+    pub async fn deploy<T: Network>(
+        self,
+        worker: &Worker<T>,
+        wasm: Vec<u8>,


I wonder if, like the other conversion from String to &str, this should also be done because in the future we can assume that we don't actually require an owned vec to be able to serialize and send over the wire?

That's the thing about a lot of the near_primitives/rpc, a lot of it requires owned types. Like DeployContractAction takes in a Vec<u8> so I can't do much here unless we change the deps

Important meta note: whatever our private deps do internally should not have any bearing on the public API we expose.

The RPC lib we use can use &[u8] or Vec<u8>, or Bytes, but that shouldn't change the public types. We should be able to refactor implementation from Vec<u8> to Bytes and back, if we like to, without making our user's change the code.

In this sense, yeah, it seems that wasm: &[u8] would be a better signature here -- nothing in the problem fundamentally requires owned vec. I can also imagine the following idiom for sandbox testing:

let CODE: &'static [u8] = include_bytes!("./target/wasm32-unknown-unknow/contrct.wasm"); #[test] fn foo() {}

Well, context is the king: in some cases, you want the abstraction to be inherently leaky to have, eg, maximal performance. But what we are doing here is essentially a scripting layer, so we have a lot of freedom in how we organize internals, and have a lot of responsibility in making the public API most natural for the user.

hmm, let's just do wasm: AsRef<[u8]> then so we can take in both Vec<u8> and &[u8]

Using AsRef in the APIs is generally a bad idea, as it makes the function signature much harder to read, and also regresses type inference at the call-site. If the user have xs: Vec<u8>, they can just pass &xs.

The only place where AsRef polymoprhism makes sense is for accepting a &Path. Note that it is needed there not to convert from owned to borrowed (PathBuf -> Path) but between borrowed types (&str -> &Path). And even than, it creates usability paper-cut, where passing in PathBuf works, but then you loose owenrship of it and can't, eg, print it in the error message.

ChaoticTempest · 2021-12-16T21:37:41Z

gonna pull this one in as it seems pretty robust in its current form, but we can still continue the conversation here

Add call builder

84536e3

This was referenced Dec 8, 2021

How to call a method on the contract and sign it by the user? #32

Closed

chore: Upgrade jsonrpc-client & do not expose near_* crate types #25

Merged

ChaoticTempest added 4 commits December 10, 2021 23:08

Change function sig of view to return Json String

c3eeb75

Added helper functions for transfer_near, delete_account, view

70b0ada

Merge branch 'main' of github.com:near/workspaces-rs into feat/call-b…

66ca52f

…uilder

Format

908c096

ChaoticTempest changed the base branch from feat/no-pub-nearcore-types to main December 10, 2021 23:15

ChaoticTempest changed the title ~~feat: convenience Account::call using call builder~~ feat: convenience functions for Contract and Account Dec 10, 2021

ChaoticTempest marked this pull request as ready for review December 10, 2021 23:21

matklad reviewed Dec 13, 2021

View reviewed changes

workspaces/src/network/account.rs Show resolved Hide resolved

Added create_account for sub account creation

45b4f60

ChaoticTempest requested a review from austinabell December 13, 2021 23:32

austinabell reviewed Dec 14, 2021

View reviewed changes

ChaoticTempest added 2 commits December 14, 2021 20:20

Add serde/borsh deser from view calls

e85e92b

Fixup examples/tests

afc0131

austinabell reviewed Dec 14, 2021

View reviewed changes

workspaces/src/network/account.rs Outdated Show resolved Hide resolved

Add serde::Serialize to with_args_json

087fdbe

ChaoticTempest mentioned this pull request Dec 14, 2021

Investigate inner JSON RPC serialization #36

Closed

matklad approved these changes Dec 15, 2021

View reviewed changes

austinabell reviewed Dec 15, 2021

View reviewed changes

ChaoticTempest added 5 commits December 15, 2021 20:33

Rename try_* to .borsh and .json

046b391

Add additional lifetimes

415f63b

Drop with_ prefix for builders

4692e13

Add #[non_exhaustive] to detail objects

94886c3

Add DeserializedOwned & result sig for json/borsh

a682405

ChaoticTempest added 3 commits December 15, 2021 22:02

Rename dev_create{,_account}

f51ff72

Move client.call defaults out

63b02c3

Update README w/ new convenience functions

b387a88

austinabell reviewed Dec 15, 2021

View reviewed changes

workspaces/src/network/account.rs Outdated Show resolved Hide resolved

ChaoticTempest added 3 commits December 16, 2021 00:20

Update examples with newer accounts API

20962ae

Format

f049255

Add convenient deploy function

e2f9526

TrevorJTClarke reviewed Dec 16, 2021

View reviewed changes

austinabell approved these changes Dec 16, 2021

View reviewed changes

workspaces/src/network/result.rs Outdated Show resolved Hide resolved

austinabell reviewed Dec 16, 2021

View reviewed changes

ChaoticTempest added 3 commits December 16, 2021 02:02

Added docs

e8b2fc5

Rename amount_yocto to amount & correct docs

74cf2d2

Have deploy take in both vec and slice types

d01fa33

ChaoticTempest merged commit ed74d20 into main Dec 16, 2021

ChaoticTempest deleted the feat/call-builder branch December 16, 2021 21:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: convenience functions for `Contract` and `Account` #33

feat: convenience functions for `Contract` and `Account` #33

ChaoticTempest commented Dec 8, 2021 •

edited

Loading

TrevorJTClarke commented Dec 13, 2021

willemneal commented Dec 13, 2021

ChaoticTempest commented Dec 13, 2021

austinabell Dec 14, 2021

ChaoticTempest Dec 14, 2021

austinabell Dec 14, 2021

ChaoticTempest Dec 14, 2021

austinabell Dec 14, 2021

matklad left a comment

matklad Dec 15, 2021

ChaoticTempest Dec 15, 2021

matklad Dec 15, 2021

ChaoticTempest Dec 15, 2021

matklad Dec 15, 2021

ChaoticTempest Dec 15, 2021

matklad Dec 16, 2021

matklad Dec 16, 2021

ChaoticTempest Dec 16, 2021

matklad Dec 15, 2021

austinabell Dec 15, 2021

austinabell Dec 15, 2021

austinabell left a comment

TrevorJTClarke Dec 16, 2021

ChaoticTempest Dec 16, 2021 •

edited

Loading

austinabell left a comment

austinabell Dec 16, 2021

ChaoticTempest Dec 16, 2021

matklad Dec 16, 2021 •

edited

Loading

matklad Dec 16, 2021 •

edited

Loading

ChaoticTempest Dec 16, 2021

matklad Dec 17, 2021

ChaoticTempest commented Dec 16, 2021

feat: convenience functions for Contract and Account #33

feat: convenience functions for Contract and Account #33

Conversation

ChaoticTempest commented Dec 8, 2021 • edited Loading

Account/Contract Convenience Functions

TrevorJTClarke commented Dec 13, 2021

willemneal commented Dec 13, 2021

ChaoticTempest commented Dec 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinabell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChaoticTempest Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

austinabell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

matklad Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChaoticTempest commented Dec 16, 2021

feat: convenience functions for `Contract` and `Account` #33

feat: convenience functions for `Contract` and `Account` #33

ChaoticTempest commented Dec 8, 2021 •

edited

Loading

ChaoticTempest Dec 16, 2021 •

edited

Loading

matklad Dec 16, 2021 •

edited

Loading

matklad Dec 16, 2021 •

edited

Loading