refactor: replace fungible token example sim tests with workspaces #699

itegulov · 2022-01-17T11:36:51Z

I had to cut a few asserts from some of the tests since workspaces does not expose some of the stuff they needed, but at the very least all of the tests have survived the migration without losing meaningfulness. I left TODOs in appropriate places for us to revisit later.

Also, I took the liberty to delete examples/heavy.rs since it was originally added in #307 to showcase sim tests performance. But lmk if you would prefer for me to keep and refactor it too.

Related to #563 #662

itegulov · 2022-01-18T06:36:25Z

examples/fungible-token/tests/workspaces.rs

+    let none: Option<bool> = None;
+    let res = contract
+        .call(&worker, "storage_deposit")
+        .args_json((account_id, none))?


@austinabell is there a way to instantiate None with an explicit generic type in one line? I have tried None: Option<bool>, but apparently type ascriptions are experimental.

Yeah, can just do () or I think Option::<bool>::None works if you want it to be clear the type (serializes no different though)

The problem with () is that it does not match the function's signature. Even if None and () do serialize the same way, as an external user reading these examples I would be confused why we are passing () instead of Option<bool>.

Option::<bool>::None works though, thanks!

austinabell

oops forgot to finalize this review, sorry if any are stale comments. Was just a quick scan until the CI passes, which seemed more workspaces/sandbox utils related

austinabell · 2022-01-17T17:07:47Z

examples/fungible-token/tests/workspaces.rs

+const TEN_THOUSAND_NEAR: u128 = 100_000_000_000_000_000_000_000_000_000;
+const ONE_HUNDRED_NEAR: u128 = 100_000_000_000_000_000_000_000_000;
+const FIFTY_NEAR: u128 = 50_000_000_000_000_000_000_000_000;


Since we have the ONE_NEAR constant in the SDK we can use it here to be more readable? Mult can be used with consts with no added cost. Or can just avoid using these constants and do that where it is used

Yeah, at first I have refactored these values to look like 10000 * ONE_NEAR, but later I found out about parse_near! so ended up with in-place parse_near!("10000 N") instead. lmk which way you prefer.

austinabell · 2022-01-17T17:09:31Z

examples/fungible-token/tests/workspaces.rs

+        .deposit(ONE_YOCTO)
+        .transact()
+        .await?;
+    assert!(matches!(res.status, FinalExecutionStatus::SuccessValue(_)));


maybe worth adding an is_success method or something equivalent to the result to abstract this away in workspaces? cc @ChaoticTempest

I agree, that would simplify my life as a workspaces user

Interesting. Normally the pattern for these was to do an into_result()? to forward the error, but I see that in this case we want to do assertions to be more explicit. Good idea overall, ~~I'll add a ticket~~ here's a PR near/near-workspaces-rs#58 for it since it's pretty small addition anyways

Well, CallExecutionDetails does not have into_result() anyway. Thanks for the PR, I will take a look shortly

examples/fungible-token/tests/workspaces.rs

austinabell · 2022-01-18T15:49:06Z

examples/fungible-token/tests/workspaces.rs

+    let none: Option<bool> = None;
+    let res = contract
+        .call(&worker, "storage_deposit")
+        .args_json((account_id, none))?


Yeah, can just do () or I think Option::<bool>::None works if you want it to be clear the type (serializes no different though)

itegulov · 2022-01-19T04:19:49Z

oops forgot to finalize this review, sorry if any are stale comments.

No worries. Indeed, I have already refactored most of the weird stuff away.

Was just a quick scan until the CI passes, which seemed more workspaces/sandbox utils related

Yeah, I am still trying to figure out why Github Actions don't want to run the downloaded sandbox binaries... There is some weirdness going on in Buildkite too, but I have not looked too deeply into that yet.

examples/fungible-token/tests/workspaces.rs

ChaoticTempest · 2022-01-19T21:33:51Z

examples/fungible-token/tests/workspaces.rs

+        .deposit(ONE_YOCTO)
+        .transact()
+        .await?;
+    assert!(matches!(res.status, FinalExecutionStatus::SuccessValue(_)));


Interesting. Normally the pattern for these was to do an into_result()? to forward the error, but I see that in this case we want to do assertions to be more explicit. Good idea overall, ~~I'll add a ticket~~ here's a PR near/near-workspaces-rs#58 for it since it's pretty small addition anyways

examples/fungible-token/tests/workspaces.rs

austinabell

LGTM, I'll wait for @ChaoticTempest approval before pulling in because he might have some tricks :)

austinabell · 2022-01-25T14:41:41Z

examples/fungible-token/tests/workspaces.rs

+    register_user(&worker, &contract, defi_contract.id()).await?;
+
+    // root invests in defi by calling `ft_transfer_call`
+    // TODO: Pack into one transaction


can you explain what you mean by this? Not completely intuitive

yeah sorry for using incorrect terminology. What I meant to say is that these two actions (fn call ft_transfer_call and fn call storage_unregister) are supposed to be batched into a single transaction according to the old simtest-powered tests. Since workspaces do not support batched transactions yet, I had to split these two actions into two separate transactions. But to be fair I do not really see why you would want to batch these two specific action other than to (maybe?) save some gas... Atomicity does not matter here I think.

I will re-phrase the wording

examples/fungible-token/tests/workspaces.rs

ChaoticTempest

looks good to me as well. Will be adding the promise results soon, so hopefully we can add in those commented out promise_error checks

examples/fungible-token/tests/workspaces.rs

austinabell · 2022-01-25T21:23:05Z

examples/fungible-token/Cargo.toml

@@ -5,8 +5,13 @@ authors = ["Near Inc <hello@nearprotocol.com>"]
 edition = "2018"

 [dev-dependencies]
+anyhow = "1.0"
+near-primitives = "0.5.0"


yeah an extension of what Phuong mentioned, this shouldn't be included

Suggested change

near-primitives = "0.5.0"

As @ChaoticTempest mentioned in the other thread I actually need this to pull FinalExecutionStatus

This seems like something that shouldn't be exposed through workspaces

Hmm, do you mean that workspaces should have its own type for expressing the call execution status?

CC @ChaoticTempest

yeah, this is not an ideal pattern for workspace users to have to import a nearcore internal crate. Not a big deal, but something that should be changed at some point

ooof, this was a pattern I wasn't expecting to cover with asserts! with FinalExecutionStatus. Think we can defer this later until a new minor version of workspaces is released and we can convert these asserts! over to using is_success calls

Think we can defer this later until a new minor version of workspaces is released and we can convert these asserts! over to using is_success calls

Yeah, we can defer for sure and is_success will help in this case, but CallExecutionDetails::status (which is a part of the public API) will still be a type from near-primitives so I think eventually we would want to replace it with something else.

# Conflicts: # examples/fungible-token/Cargo.lock # examples/fungible-token/res/defi.wasm # examples/fungible-token/res/fungible_token.wasm

refactor: replace fungible token example sim tests with workspaces

4d62cdb

itegulov requested review from ChaoticTempest and austinabell January 17, 2022 11:36

refactor initialization

7183ff2

itegulov commented Jan 18, 2022

View reviewed changes

itegulov added 3 commits January 19, 2022 11:31

refactor some workspaces usage to be more canonical

eb26d03

Merge branch 'master' into daniyar/fungible-token-example-workspaces

03fc85d

replace near constants with parse_near! macro

7c2d4c1

austinabell reviewed Jan 19, 2022

View reviewed changes

use Option::<T>::None for creating Nones

eee6eb2

ChaoticTempest reviewed Jan 19, 2022

View reviewed changes

ChaoticTempest mentioned this pull request Jan 19, 2022

Added is_success / is_failure for CallExecution results near/near-workspaces-rs#58

Merged

itegulov added 3 commits January 20, 2022 09:24

address comments

37aafef

use a snapshot version of workspaces

f6f9e54

use workspaces 0.1.1

1688ecb

austinabell approved these changes Jan 25, 2022

View reviewed changes

ChaoticTempest approved these changes Jan 25, 2022

View reviewed changes

examples/fungible-token/tests/workspaces.rs Outdated Show resolved Hide resolved

austinabell reviewed Jan 25, 2022

View reviewed changes

itegulov added 3 commits January 27, 2022 16:10

Merge branch 'master' into daniyar/fungible-token-example-workspaces

3d3a5b8

# Conflicts: # examples/fungible-token/Cargo.lock # examples/fungible-token/res/defi.wasm # examples/fungible-token/res/fungible_token.wasm

address comments

b312fb1

re-generate wasm files

1af263a

itegulov force-pushed the daniyar/fungible-token-example-workspaces branch from 3a913d8 to 1af263a Compare January 27, 2022 05:19

itegulov merged commit 5dee86c into master Jan 27, 2022

itegulov deleted the daniyar/fungible-token-example-workspaces branch January 27, 2022 05:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: replace fungible token example sim tests with workspaces #699

refactor: replace fungible token example sim tests with workspaces #699

itegulov commented Jan 17, 2022

itegulov Jan 18, 2022

austinabell Jan 18, 2022

itegulov Jan 19, 2022

austinabell left a comment

austinabell Jan 17, 2022

itegulov Jan 19, 2022

austinabell Jan 17, 2022

itegulov Jan 19, 2022

ChaoticTempest Jan 19, 2022 •

edited

Loading

itegulov Jan 19, 2022

austinabell Jan 18, 2022

itegulov commented Jan 19, 2022

ChaoticTempest Jan 19, 2022 •

edited

Loading

austinabell left a comment

austinabell Jan 25, 2022

itegulov Jan 25, 2022 •

edited

Loading

ChaoticTempest left a comment

austinabell Jan 25, 2022

itegulov Jan 26, 2022

austinabell Jan 27, 2022

itegulov Jan 27, 2022

austinabell Jan 27, 2022

ChaoticTempest Jan 27, 2022

itegulov Jan 27, 2022

refactor: replace fungible token example sim tests with workspaces #699

refactor: replace fungible token example sim tests with workspaces #699

Conversation

itegulov commented Jan 17, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChaoticTempest Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itegulov commented Jan 19, 2022

ChaoticTempest Jan 19, 2022 • 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

itegulov Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

ChaoticTempest 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

ChaoticTempest Jan 19, 2022 •

edited

Loading

ChaoticTempest Jan 19, 2022 •

edited

Loading

itegulov Jan 25, 2022 •

edited

Loading