synchronous makeVstorageKit #10670

turadg · 2024-12-11T02:08:59Z

incidental

Description

In the course of working on agoric-labs/vstorage-viewer#3 it was hard to work with an async makeVstorageKit. It was async because construction of the kit required fetching a bunch of data from vstorage to populate agoricNames.

That concern is a higher level than vstorage so this pulls it out. That allows makeVstorageKit to be synchronous.

I added a test of makeAgoricNames both before and after.

Eventually I think something should make it easy to make an AgoricNamesKit that wraps VstorageKit the way VstorageKit wraps Vstorage. makeWalletUtils does that but needs more work. At least renaming to SmartWallet but it would help to have a rethink of the layering and vstorage decoding in client-utils.

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

not documented but this will make it less complex

Testing Considerations

new test

While refactoring, tsc really let me down by not detecting destuctured properies that were no longer on the source object. I suppose our settings aren't strict enough. I enabled noImplicitAny to detect those (by then filtering for makeVstorageKit)

Upgrade Considerations

n/a

cloudflare-workers-and-pages · 2024-12-11T03:42:25Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`501be3a`
Status:	✅ Deploy successful!
Preview URL:	https://89f8ac69.agoric-sdk.pages.dev
Branch Preview URL:	https://ta-sync-vstoragekit.agoric-sdk.pages.dev

View logs

samsiegart

The CI failure looks like it might be related to the changes, will TAL after that's passing

samsiegart

LGTM mostly but please see my non-blocking request to add a comment

samsiegart · 2024-12-11T18:56:15Z

a3p-integration/proposals/s:stake-bld/test-lib/wallet.js

+/**
+ * Stop-gap using execFileSync until we have a pure JS signing client.
+ *
+ * @param {object} root0


Question: this root0 seems a little out of place, yet familiar... is this codegen or something?

it's what the eslint autofixer for missing jsdoc params does

samsiegart · 2024-12-11T19:01:08Z

a3p-integration/proposals/z:acceptance/test-lib/kread.js

@@ -1,4 +1,5 @@
 // @ts-nocheck FIXME
+// XXX uses agoric.follow to read data through spawned processes; replace with VstorageKit


Question: is agoric.follow deprecated? Just not preferred? Is there a larger pattern of work to replace it with vstorage kit that we should file?

Deprecated, yeah. The larger pattern is in the comment, "replace with VstorageKit". That gets the vstorage data without shelling out, making it more portable and less dependent on the process env (e.g. PATH)

samsiegart · 2024-12-11T19:05:12Z

packages/client-utils/test/snapshots/vstorage-kit.test.js.md

@@ -0,0 +1,352 @@
+# Snapshot report for `test/vstorage-kit.test.js`


samsiegart · 2024-12-11T19:13:12Z

packages/fast-usdc/src/cli/lp-commands.js

@@ -50,7 +55,7 @@ const parseUSDCAmount = (amountString, usdc) => {
 */
 export const addLPCommands = (
  program,
-  { fetch, vstorageKit, stderr, stdout, env, now },
+  { fetch, agoricNames, vstorageKit, stderr, stdout, env, now },


It seems a little fickle to inject both agoricNames and vstorageKit because agoricNames should always come from the vstorageKit's queries. Being able to independently craft them allows them to have conflicting results. I.e. vstorageKit could return something when querying published.agoricNames.brands that doesn't match the agoricNames arg. It would be helpful to at least document these options as test-only to avoid confusion. So request: please add a doc/comment saying these are for testing.

It seems a little fickle to inject both agoricNames and vstorageKit because agoricNames should always come from the vstorageKit's queries

Not anymore, right? The PR is about removing agoricNames from VstorageKit.

I thought this PR is about making vstorageKit synchronous. If there's a context where looking at a provided agoricNames object contradicts what would result by calling the provided vstorageKit.readPublished("agoricNames...") then I think that's a bad consequence. Perhaps make vstorageKit.agoricNames a promise instead of removing it then?

Perhaps make vstorageKit.agoricNames a promise instead of removing it then?

I don't think it's necessary to do this. My main concern is that we have a function with two arguments that can be two different sources of truth for the same data, it would just be nice to highlight this hazard somehow with a comment.

My main concern is that we have a function with two arguments that can be two different sources of truth for the same data, it would just be nice to highlight this hazard somehow with a comment.

Oh, I see. Yes that is a hazard. I'll work on that.

turadg requested review from samsiegart and 0xpatrickdev December 11, 2024 02:08

turadg requested a review from a team as a code owner December 11, 2024 02:09

turadg added the force:integration Force integration tests to run on PR label Dec 11, 2024

samsiegart requested changes Dec 11, 2024

View reviewed changes

chore(types): stdout optional

a3872d0

turadg force-pushed the ta/sync-VstorageKit branch from 614397f to bb7daca Compare December 11, 2024 17:32

turadg added 3 commits December 11, 2024 09:41

chore(types): add typescript

e115474

test: vstorageKit.agoricNames

10302ad

chore!: remove agoricNames from VstorageKit

1c69d39

turadg force-pushed the ta/sync-VstorageKit branch from bb7daca to a9b385a Compare December 11, 2024 17:46

turadg requested a review from samsiegart December 11, 2024 18:36

samsiegart approved these changes Dec 11, 2024

View reviewed changes

turadg added 6 commits December 11, 2024 12:35

refactor: s/makeWalletUtils/makeSmartWalletKit

15976b5

refactor: fu cmds with SmartWalletKit

b7180b0

refactor: backwards-compat for wallet-utils

fdcf70c

test: replace rpc.js with client-utils

97e431f

test: DRYer z:acceptance test-lib

36a986b

test: base makeAgdWalletUtils on SmartWalletKit

501be3a

turadg force-pushed the ta/sync-VstorageKit branch from a9b385a to 501be3a Compare December 11, 2024 20:51

turadg added the automerge:rebase Automatically rebase updates, then merge label Dec 11, 2024

mergify bot merged commit 8f9f075 into master Dec 11, 2024
86 of 95 checks passed

mergify bot deleted the ta/sync-VstorageKit branch December 11, 2024 21:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

synchronous makeVstorageKit #10670

synchronous makeVstorageKit #10670

turadg commented Dec 11, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 11, 2024 •

edited

Loading

samsiegart left a comment

samsiegart left a comment

samsiegart Dec 11, 2024

turadg Dec 11, 2024

samsiegart Dec 11, 2024

turadg Dec 11, 2024

samsiegart Dec 11, 2024

samsiegart Dec 11, 2024

turadg Dec 11, 2024

samsiegart Dec 11, 2024

samsiegart Dec 11, 2024

turadg Dec 11, 2024

turadg Dec 11, 2024

		@@ -1,4 +1,5 @@
		// @ts-nocheck FIXME
		// XXX uses agoric.follow to read data through spawned processes; replace with VstorageKit

		@@ -0,0 +1,352 @@
		# Snapshot report for `test/vstorage-kit.test.js`

synchronous makeVstorageKit #10670

synchronous makeVstorageKit #10670

Conversation

turadg commented Dec 11, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Dec 11, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

samsiegart left a comment

Choose a reason for hiding this comment

samsiegart 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

turadg commented Dec 11, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 11, 2024 •

edited

Loading