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

adsick · 2022-04-21T08:04:44Z

Hi, I'm working on #12 and #19. Now I feel like some review of what I've done would be great, please tell me what API is preferable and how to shape it (where to put stuff etc.).

Highlights:

Improve over refactor: change patch state parameters to be slices #80
now accepting wider variety of types by using Into<Vec<u8>>

    pub async fn patch_state(
        &self,
        contract_id: &AccountId,
        key: impl Into<Vec<u8>>,
        value: impl Into<Vec<u8>>,
    )

Implemented Allow patch_state to take in multiple patches #12

    pub async fn patch_state_multiple(
        &self,
        account_id: &AccountId,
        kvs: impl IntoIterator<Item = (impl Into<Vec<u8>>, impl Into<Vec<u8>>)>,
    )

Implemented Support patching account and access-keys #19 by adapting feat: patch account and access keys added #122 to be a builder
(for more see line 235 of sandbox.rs)

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

bonus: fixed long cargo check/build times issue by not installing the node every time - the issue was in the build.rs

Now I'm looking for some feedback to refine API 🙂

now using ensure_sandbox_bin() instead of installing it every time

workspaces/src/network/sandbox.rs

ChaoticTempest

Thanks so much for getting this implementation in! Did a cursory look and left a few comments so we can improve the maintainability of it

workspaces/src/network/mod.rs

ChaoticTempest · 2022-04-29T20:34:46Z

workspaces/build.rs

+        // 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");


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");

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?

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?

ChaoticTempest · 2022-04-29T20:36:15Z

workspaces/src/network/sandbox.rs

+    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(


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

Yeah, we didn't tested because we waited for the feedback. Tests will be added too.

workspaces/src/network/sandbox.rs

ChaoticTempest · 2022-04-29T20:54:05Z

workspaces/src/worker/impls.rs

+    pub async fn patch_state_multiple(
+        &self,
+        account_id: &AccountId,
+        kvs: impl IntoIterator<Item = (impl Into<Vec<u8>>, impl Into<Vec<u8>>)>,
    ) -> anyhow::Result<()> {


I don't know if I'm a fan of taking an IntoIterator as a parameter, but this might be the only way I can think of doing this.

@matklad @austinabell, would you have to know of anything better than this? Or if there's a better API design than having a separate function just be for the iterator

yeah, for the same rationale as #124 (comment) but primarily that the API will transition away from requiring owned Vec<u8> and will be a breaking change down the road to change. Prob best to constrain to &[u8] for now

You want impl std::iter::Extend here.

workspaces/src/network/sandbox.rs

ChaoticTempest · 2022-04-29T20:59:34Z

workspaces/src/network/sandbox.rs

+    //     self
+    // }
+
+    pub async fn apply(self) -> anyhow::Result<()> {


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

ChaoticTempest · 2022-04-29T21:00:53Z

workspaces/src/worker/impls.rs

+        key: impl Into<Vec<u8>>,
+        value: impl Into<Vec<u8>>,


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.

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>

ChaoticTempest · 2022-04-29T21:01:49Z

workspaces/src/worker/impls.rs

+    /// 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 {


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

Let's merge this one and the other

which 'other' one?

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

matklad · 2022-05-02T17:22:41Z

workspaces/src/network/sandbox.rs

+        }
+    }
+
+    pub fn data(mut self, key: impl Into<Vec<u8>>, value: impl Into<Vec<u8>>) -> Self {


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.

matklad · 2022-05-02T17:22:55Z

workspaces/src/network/sandbox.rs

+    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(),
+        }));
+        self
+    }


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<'_> { }

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

good point about the builder!

Indeed, to look at the std, theer's

https://doc.rust-lang.org/stable/std/process/struct.Command.html#method.arg

https://doc.rust-lang.org/stable/std/process/struct.Command.html#method.args

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 :)

+1 for .entry and .entries

+1 for .entry and .entries

got it

matklad · 2022-05-02T17:25:12Z

workspaces/src/network/sandbox.rs

+        }
+    }
+
+    pub fn data(mut self, key: impl Into<Vec<u8>>, value: impl Into<Vec<u8>>) -> Self {


Seems like we should have a way to delete key as well?

matklad · 2022-05-02T17:27:18Z

workspaces/src/network/sandbox.rs

+        records.extend(kvs.into_iter().map(|(key, value)| StateRecord::Data {
+            account_id: account_id.clone(),
+            data_key: key.into(),
+            value: value.into(),
+        }));


Minor: there's logical duplication between the two methods, one could delegate to the other (or both could delegate to internal _state_record)

matklad · 2022-05-02T17:29:43Z

workspaces/src/worker/impls.rs

+    pub async fn patch_state_multiple(
+        &self,
+        account_id: &AccountId,
+        kvs: impl IntoIterator<Item = (impl Into<Vec<u8>>, impl Into<Vec<u8>>)>,
    ) -> anyhow::Result<()> {


You want impl std::iter::Extend here.

adsick · 2022-05-03T20:31:18Z

Thanks a bunch for the feedback, @matklad 's guidance is looking great. This week we'll try to update this PR accordingly and then start testing when that's ok in terms of API and stuff.

removed comments, Co-authored-by: Phuong Nguyen <ChaoticTempest@users.noreply.github.com>

https://github.com/adsick/workspaces-rs into FIRT-13-allow-patch-state-to-take-in-multiple-patches

Co-authored-by: Phuong Nguyen <ChaoticTempest@users.noreply.github.com>

…tate-to-take-in-multiple-patches

adsick added 9 commits April 19, 2022 13:19

patch state builder init

94d66ac

fix build.rs

bda570d

now using ensure_sandbox_bin() instead of installing it every time

builder pattern progress1

74c4f4f

docfix patch_state_many

f351e76

renamed .send() to .apply()

0d2167d

refactor to pass AccountId once + access_key patch

58fa329

patch_account with builder pattern

6a99349

remove some comments

991ce43

remove some comments

7ff6b82

Buckram123 mentioned this pull request Apr 21, 2022

feat: patch account and access keys added #122

Closed

Buckram123 reviewed Apr 21, 2022

View reviewed changes

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

Buckram123 added 2 commits April 21, 2022 15:16

feat: access key builder

2ec00fc

style: renamed it to match crate function

edceb77

ChaoticTempest requested changes Apr 29, 2022

View reviewed changes

matklad reviewed May 2, 2022

View reviewed changes

adsick and others added 7 commits May 5, 2022 23:17

suggestions 1, 2

50d2266

changed unwrap to expect in workspaces/build.rs

edacfd6

removed comments, Co-authored-by: Phuong Nguyen <ChaoticTempest@users.noreply.github.com>

Merge branch 'FIRT-13-allow-patch-state-to-take-in-multiple-patches' of

f8842e3

https://github.com/adsick/workspaces-rs into FIRT-13-allow-patch-state-to-take-in-multiple-patches

explanatory errors in build.rs

1c4c459

removed commented access_key method in workspaces/src/network/sandbox.rs

7e1b304

Co-authored-by: Phuong Nguyen <ChaoticTempest@users.noreply.github.com>

replace StorageUsage and use slices

ff26068

Merge remote-tracking branch 'origin/main' into FIRT-13-allow-patch-s…

132938f

…tate-to-take-in-multiple-patches

ChaoticTempest mentioned this pull request Dec 6, 2022

New patch state, key, account API #250

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

adsick commented Apr 21, 2022 •

edited

Loading

ChaoticTempest left a comment

ChaoticTempest Apr 29, 2022

adsick May 1, 2022

ChaoticTempest May 11, 2022

ChaoticTempest Apr 29, 2022

adsick May 1, 2022

ChaoticTempest Apr 29, 2022

austinabell May 2, 2022

matklad May 2, 2022

ChaoticTempest Apr 29, 2022

ChaoticTempest Apr 29, 2022

austinabell May 2, 2022

ChaoticTempest Apr 29, 2022

adsick May 1, 2022

ChaoticTempest May 11, 2022

matklad May 2, 2022

matklad May 2, 2022

adsick May 6, 2022 •

edited

Loading

matklad May 11, 2022

ChaoticTempest May 11, 2022

adsick May 12, 2022

matklad May 2, 2022

matklad May 2, 2022

matklad May 2, 2022

adsick commented May 3, 2022 •

edited

Loading

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

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

Are you sure you want to change the base?

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

Conversation

adsick commented Apr 21, 2022 • edited Loading

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

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

adsick May 6, 2022 • edited Loading

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

adsick commented May 3, 2022 • edited Loading

adsick commented Apr 21, 2022 •

edited

Loading

adsick May 6, 2022 •

edited

Loading

adsick commented May 3, 2022 •

edited

Loading