refactor StorageNode type #5945

turadg · 2022-08-12T19:19:33Z

Description

We have ChainStorageNode and StorageNode but it seems the latter is sufficient. The only difference is the former has clearValue, which is the same as setValue(undefined).

This removes clearValue and replaces ChainStorageNode with StorageNode.

It also renames the makeStorageNode function to distinguish its use case (making a child) and makes childName required.

Security Considerations

--

Documentation Considerations

--

Testing Considerations

CI

gibson042

I like the overall direction, but would prefer to keep the ChainStorageNode type rather than StorageNode (to indicate the dependency upon chain storage semantics).

Also, since you're doing this, how about moving the type definition from packages/notifier/src/types.js to a more logical home, and also providing a definition of what instances of the type represent (e.g., "a node in a hierarchical externally-reachable storage structure that identifies children by restricted ASCII name and is associated with arbitrary string-valued data defaulting to the empty string")?

gibson042 · 2022-08-12T19:26:17Z

packages/vats/src/lib-chainStorage.js

 * @returns {Promise<StorageNode>}
 */
-export async function makeStorageNode(chainStorage, childName) {
+export async function makeStorageNodeChild(chainStorage, childName) {


I support this rename, and would recommend taking it farther:

Suggested change

export async function makeStorageNodeChild(chainStorage, childName) {

export async function makeStorageNodeChild(storageNode, childName) {

packages/vats/src/lib-chainStorage.js

gibson042 · 2022-08-12T19:44:45Z

packages/vats/src/lib-chainStorage.js

-      clearValue() {
-        handleStorageMessage({ key: path, method: 'set' });
-      },


This is a hard pill to swallow because there ought to be a conceptual difference between "set to empty" vs. "unset", but factually that is not the case in vstorage.go. So I do not object.

gibson042 · 2022-08-12T19:48:51Z

packages/vats/test/test-lib-chainStorage.js

@@ -68,7 +75,7 @@ test('makeChainStorageRoot', async t => {
    );
  }

-  rootNode.clearValue();
+  rootNode.setValue(undefined);


This call is invalid.

Suggested change

rootNode.setValue(undefined);

rootNode.setValue('');

gibson042 · 2022-08-12T19:49:41Z

packages/vats/test/test-lib-chainStorage.js

@@ -172,22 +173,22 @@ test('makeChainStorageRoot', async t => {
    'level-skipping setValue message',
  );

-  childNode.clearValue();
+  childNode.setValue(undefined);


Suggested change

childNode.setValue(undefined);

childNode.setValue('');

gibson042 · 2022-08-12T19:49:54Z

packages/vats/test/test-lib-chainStorage.js

  );
-  deepNode.clearValue();
+  deepNode.setValue(undefined);


Suggested change

deepNode.setValue(undefined);

deepNode.setValue('');

gibson042 · 2022-08-12T19:50:05Z

packages/vats/test/test-lib-chainStorage.js

  );
-  childNode.clearValue();
+  childNode.setValue(undefined);


Suggested change

childNode.setValue(undefined);

childNode.setValue('');

gibson042

Thanks!

gibson042 · 2022-08-15T02:28:43Z

packages/notifier/src/types.js

+ *
+ * Vstorage is a hierarchical externally-reachable storage structure that
+ * identifies children by restricted ASCII name and is associated with arbitrary
+ * string-valued data defaulting to the empty string.


Suggested change

* string-valued data defaulting to the empty string.

* string-valued data for each node, defaulting to the empty string.

gibson042 · 2022-08-15T02:35:05Z

packages/vats/src/lib-chainStorage.js

      getStoreKey() {
        return handleStorageMessage({ key: path, method: 'getStoreKey' });
      },
-      getChildNode(name) {
+      /** @type {(name: string) => VStorageNode} */
+      makeChildNode(name) {


In Agoric conventions that I learned in the past few weeks, I think the most appropriate name for this method is provideChildNode ("provide" referring to operations that return a value after creating it if necessary, acknowledging that it might have already existed).

And likewise for makeStorageNodeChild.

I think with provide you get the same object on repeated requests. The two methods you suggest renaming actually call makeChainStorageNode which always makes a fresh node. Changing that is out of scope but I can follow up to do that. WDYT @michaelfig ?

turadg · 2022-08-15T15:36:28Z

I just noticed

agoric-sdk/packages/casting/src/casting-spec.js

Lines 44 to 48 in a457f75

    
           export const pathPrefixToConverters = harden({ 
        
             'swingset:': swingsetPathToCastingSpec, 
        
             'vstore:': vstoragePathToCastingSpec, 
        
             ':': DEFAULT_PATH_CONVERTER, 
        
           });

and now I'm not so sure about removing StorageNode.

@michaelfig are there meant to be more than one backing of StorageNode? How do swingset and vstore relate? Is it that swingset is chain and vstore is off-chain?

turadg · 2022-08-16T19:08:29Z

@Mergifyio unqueue

mergify · 2022-08-16T19:08:42Z

unqueue

✅ The pull request has been removed from the queue

mhofman · 2022-08-16T19:41:33Z

@Mergifyio queue

mergify · 2022-08-16T19:42:06Z

Sorry but I didn't understand the arguments of the command queue. Please consult the commands documentation 📚.

mhofman · 2022-08-16T19:42:42Z

@Mergifyio refresh

mergify · 2022-08-16T19:42:59Z

refresh

✅ Pull request refreshed

mhofman · 2022-08-16T19:46:00Z

@Mergifyio queue master

mergify · 2022-08-16T19:46:34Z

Sorry but I didn't understand the arguments of the command queue. Please consult the commands documentation 📚.

turadg requested review from michaelfig and gibson042 August 12, 2022 19:19

turadg mentioned this pull request Aug 12, 2022

feat!: Support publishing and consuming chain storage stream cells #5942

Merged

gibson042 requested changes Aug 12, 2022

View reviewed changes

turadg force-pushed the ta/storageNode-cleanup branch from 86e7bfd to 48754ea Compare August 12, 2022 23:50

turadg requested a review from dtribble as a code owner August 12, 2022 23:50

gibson042 approved these changes Aug 15, 2022

View reviewed changes

turadg force-pushed the ta/storageNode-cleanup branch from 8e4cad2 to a656966 Compare August 15, 2022 20:51

turadg added the automerge:rebase Automatically rebase updates, then merge label Aug 15, 2022

turadg force-pushed the ta/storageNode-cleanup branch 5 times, most recently from 37430c8 to e1e196c Compare August 16, 2022 18:54

turadg removed the automerge:rebase Automatically rebase updates, then merge label Aug 16, 2022

mhofman added the automerge:rebase Automatically rebase updates, then merge label Aug 16, 2022

turadg force-pushed the ta/storageNode-cleanup branch from e1e196c to 7d56fa8 Compare August 16, 2022 20:33

turadg removed the automerge:rebase Automatically rebase updates, then merge label Aug 16, 2022

turadg added 3 commits August 16, 2022 16:25

BREAKING CHANGE: remove clearValue() from ChainStorageNode

45390ac

chore: remove ChainStorageNode type

b6ca9d0

refactor: makeStorageNode → makeStorageNodeChild

10a2f9f

turadg added 4 commits August 16, 2022 16:25

refactor(storageNode): storeNodeRef param

0a899aa

refactor(StorageNode): getChildNode → makeChildNode

e00c125

chore(notifier): define VStorageKey type

216ba7c

refactor(notifier): remove getStoreKey from StoredSubscription

a67d808

turadg force-pushed the ta/storageNode-cleanup branch from 7d56fa8 to a67d808 Compare August 16, 2022 23:33

turadg added the automerge:rebase Automatically rebase updates, then merge label Aug 17, 2022

turadg changed the title ~~remove ChainStorageNode type~~ refactor StorageNode type Aug 17, 2022

mergify bot merged commit c81ad73 into master Aug 17, 2022

mergify bot deleted the ta/storageNode-cleanup branch August 17, 2022 14:58

turadg mentioned this pull request Apr 13, 2023

block-aware API for deleting StorageNodes #7405

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor StorageNode type #5945

refactor StorageNode type #5945

turadg commented Aug 12, 2022

gibson042 left a comment •

edited

Loading

gibson042 Aug 12, 2022

gibson042 Aug 12, 2022

gibson042 Aug 12, 2022

gibson042 Aug 12, 2022

gibson042 Aug 12, 2022

gibson042 Aug 12, 2022

gibson042 left a comment

gibson042 Aug 15, 2022

gibson042 Aug 15, 2022

turadg Aug 15, 2022

turadg commented Aug 15, 2022

turadg commented Aug 16, 2022

mergify bot commented Aug 16, 2022

mhofman commented Aug 16, 2022

mergify bot commented Aug 16, 2022

mhofman commented Aug 16, 2022

mergify bot commented Aug 16, 2022

mhofman commented Aug 16, 2022

mergify bot commented Aug 16, 2022

	export async function makeStorageNodeChild(chainStorage, childName) {
	export async function makeStorageNodeChild(storageNode, childName) {

	* string-valued data defaulting to the empty string.
	* string-valued data for each node, defaulting to the empty string.

refactor StorageNode type #5945

refactor StorageNode type #5945

Conversation

turadg commented Aug 12, 2022

Description

Security Considerations

Documentation Considerations

Testing Considerations

gibson042 left a comment • 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

gibson042 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

turadg commented Aug 15, 2022

turadg commented Aug 16, 2022

mergify bot commented Aug 16, 2022

✅ The pull request has been removed from the queue

mhofman commented Aug 16, 2022

mergify bot commented Aug 16, 2022

mhofman commented Aug 16, 2022

mergify bot commented Aug 16, 2022

✅ Pull request refreshed

mhofman commented Aug 16, 2022

mergify bot commented Aug 16, 2022

gibson042 left a comment •

edited

Loading