From cdd9b82eadd18f40608c1b9cd8109589f95bcbba Mon Sep 17 00:00:00 2001 From: Richard Gibson Date: Tue, 18 Jul 2023 19:06:46 -0400 Subject: [PATCH 1/4] feat(internal): Add StorageNode `delete` method Fixes #7405 --- golang/cosmos/x/vstorage/vstorage.go | 33 +++++++++++ golang/cosmos/x/vstorage/vstorage_test.go | 59 +++++++++++++++---- packages/internal/src/lib-chainStorage.js | 24 +++++--- packages/internal/src/storage-test-utils.js | 9 +++ .../internal/test/test-storage-test-utils.js | 32 ++++++++++ 5 files changed, 136 insertions(+), 21 deletions(-) diff --git a/golang/cosmos/x/vstorage/vstorage.go b/golang/cosmos/x/vstorage/vstorage.go index b2120948a30..e82aa966c3a 100644 --- a/golang/cosmos/x/vstorage/vstorage.go +++ b/golang/cosmos/x/vstorage/vstorage.go @@ -31,6 +31,16 @@ func NewStorageHandler(keeper Keeper) vstorageHandler { return vstorageHandler{keeper: keeper} } +func unmarshalString(jsonText json.RawMessage) (string, error) { + var str *string + if err := json.Unmarshal(jsonText, &str); err != nil { + return "", err + } else if str == nil { + return "", fmt.Errorf("cannot unmarshal `null` into string") + } + return *str, nil +} + func unmarshalSinglePathFromArgs(args []json.RawMessage, path *string) error { if len(args) == 0 { return fmt.Errorf("missing 'path' argument") @@ -41,6 +51,18 @@ func unmarshalSinglePathFromArgs(args []json.RawMessage, path *string) error { return json.Unmarshal(args[0], path) } +func unmarshalPathsFromArgs(args []json.RawMessage) ([]string, error) { + paths := make([]string, len(args)) + for i, arg := range args { + path, err := unmarshalString(arg) + if err != nil { + return nil, err + } + paths[i] = path + } + return paths, nil +} + func (sh vstorageHandler) Receive(cctx *vm.ControllerContext, str string) (ret string, err error) { keeper := sh.keeper msg := new(vstorageMessage) @@ -104,6 +126,17 @@ func (sh vstorageHandler) Receive(cctx *vm.ControllerContext, str string) (ret s } return "true", nil + case "delete": + paths, err := unmarshalPathsFromArgs(msg.Args) + if err != nil { + return "", err + } + for _, path := range paths { + newEntry := types.NewStorageEntryWithNoData(path) + keeper.SetStorageAndNotify(cctx.Context, newEntry) + } + return "true", nil + case "append": for _, arg := range msg.Args { var entry types.StorageEntry diff --git a/golang/cosmos/x/vstorage/vstorage_test.go b/golang/cosmos/x/vstorage/vstorage_test.go index 02e478aea09..4b21fe01520 100644 --- a/golang/cosmos/x/vstorage/vstorage_test.go +++ b/golang/cosmos/x/vstorage/vstorage_test.go @@ -123,12 +123,13 @@ func TestGetAndHas(t *testing.T) { } } -func doTestSet(t *testing.T, method string, expectNotify bool) { +func doTestSetAndDelete(t *testing.T, setMethod string, expectNotify bool) { kit := makeTestKit() keeper, handler, ctx, cctx := kit.keeper, kit.handler, kit.ctx, kit.cctx type testCase struct { label string + method string args []interface{} errContains *string skipReadBack map[int]bool @@ -144,11 +145,20 @@ func doTestSet(t *testing.T, method string, expectNotify bool) { args: []interface{}{[]string{"baz.a", "qux"}, []string{"baz.b", "qux"}}, }, {label: "other multi-value", - args: []interface{}{[]string{"qux", "A"}, []string{"quux", "B"}}, + args: []interface{}{[]string{"qux", "A"}, []string{"quux", "B"}, []string{"ephemeral", "C"}}, }, {label: "overwrites", args: []interface{}{[]string{"bar"}, []string{"quux", "new"}}, }, + {label: "deletes", method: "delete", + args: []interface{}{"final.final.corge", "baz.b", "baz.a"}, + }, + {label: "more deletes", method: "delete", + args: []interface{}{"ephemeral", "notfound", "alsonotfound"}, + }, + {label: "restorations", + args: []interface{}{[]string{"baz.b", "quux"}, []string{"baz.a", "quux"}}, + }, {label: "non-string path", // TODO: Fully validate input before making changes // args: []interface{}{[]string{"foo", "X"}, []interface{}{42, "new"}}, @@ -161,6 +171,10 @@ func doTestSet(t *testing.T, method string, expectNotify bool) { args: []interface{}{[]interface{}{"foo", true}}, errContains: ptr("value"), }, + {label: "null path", method: "delete", + args: []interface{}{nil, "foo"}, + errContains: ptr("null"), + }, {label: "self-overwrite", args: []interface{}{[]string{"final.final.corge", "grault"}, []string{"final.final.corge", "garply"}}, skipReadBack: map[int]bool{0: true}, @@ -175,14 +189,31 @@ func doTestSet(t *testing.T, method string, expectNotify bool) { } // Expect events to be alphabetized by key. expectedFlushEvents := sdk.Events{ - stateChangeEvent("baz.a", "qux"), - stateChangeEvent("baz.b", "qux"), + stateChangeEvent("baz.a", "quux"), + stateChangeEvent("baz.b", "quux"), stateChangeEvent("final.final.corge", "garply"), stateChangeEvent("foo", "bar"), stateChangeEvent("quux", "new"), stateChangeEvent("qux", "A"), } for _, desc := range cases { + method := desc.method + if method == "" { + method = setMethod + } else if method == "delete" && setMethod == "setWithoutNotify" { + // "delete" notifies, so replace it with set-empty when testing *without* notifies. + method = setMethod + for i, arg := range desc.args { + if str, ok := arg.(string); ok { + desc.args[i] = []string{str} + } else { + desc.args[i] = []interface{}{arg} + if desc.errContains != nil { + desc.errContains = ptr("path") + } + } + } + } got, err := callReceive(handler, cctx, method, desc.args) if desc.errContains == nil { @@ -195,6 +226,10 @@ func doTestSet(t *testing.T, method string, expectNotify bool) { // Read the data back. for i, arg := range desc.args { entry, ok := arg.([]string) + if method == "delete" { + path, pathOk := arg.(string) + entry, ok = []string{path}, pathOk + } if desc.skipReadBack[i] { continue } else if !ok { @@ -224,12 +259,12 @@ func doTestSet(t *testing.T, method string, expectNotify bool) { // Verify corresponding events. if got := ctx.EventManager().Events(); !reflect.DeepEqual(got, sdk.Events{}) { - t.Errorf("%s got unexpected events before flush %#v", method, got) + t.Errorf("%s got unexpected events before flush %#v", setMethod, got) } keeper.FlushChangeEvents(ctx) if !expectNotify { if got := ctx.EventManager().Events(); !reflect.DeepEqual(got, sdk.Events{}) { - t.Errorf("%s got unexpected events after flush %#v", method, got) + t.Errorf("%s got unexpected events after flush %#v", setMethod, got) } } else if got := ctx.EventManager().Events(); !reflect.DeepEqual(got, expectedFlushEvents) { for _, evt := range got { @@ -237,18 +272,18 @@ func doTestSet(t *testing.T, method string, expectNotify bool) { for i, attr := range evt.Attributes { attrs[i] = fmt.Sprintf("%s=%q", attr.Key, attr.Value) } - t.Logf("%s got event %s<%s>", method, evt.Type, strings.Join(attrs, ", ")) + t.Logf("%s got event %s<%s>", setMethod, evt.Type, strings.Join(attrs, ", ")) } - t.Errorf("%s got after flush events %#v; want %#v", method, got, expectedFlushEvents) + t.Errorf("%s got after flush events %#v; want %#v", setMethod, got, expectedFlushEvents) } } -func TestSet(t *testing.T) { - doTestSet(t, "set", true) +func TestSetAndDelete(t *testing.T) { + doTestSetAndDelete(t, "set", true) } -func TestSetWithoutNotify(t *testing.T) { - doTestSet(t, "setWithoutNotify", false) +func TestSetWithoutNotifyAndDelete(t *testing.T) { + doTestSetAndDelete(t, "setWithoutNotify", false) } // TODO: TestAppend diff --git a/packages/internal/src/lib-chainStorage.js b/packages/internal/src/lib-chainStorage.js index 9f0e53f10db..2f08f69c24c 100644 --- a/packages/internal/src/lib-chainStorage.js +++ b/packages/internal/src/lib-chainStorage.js @@ -36,7 +36,8 @@ const { Fail } = assert; * string-valued data for each node, defaulting to the empty string. * * @typedef {object} StorageNode - * @property {(data: string) => Promise} setValue publishes some data + * @property {(data: string) => Promise} setValue publishes some data to chain storage + * @property {() => Promise} delete removes the chain storage data * @property {() => string} getPath the chain storage path at which the node was constructed * @property {() => Promise} getStoreKey DEPRECATED use getPath * @property {(subPath: string, options?: {sequence?: boolean}) => StorageNode} makeChildNode @@ -44,6 +45,7 @@ const { Fail } = assert; const ChainStorageNodeI = M.interface('StorageNode', { setValue: M.callWhen(M.string()).returns(), + delete: M.callWhen().returns(), getPath: M.call().returns(M.string()), getStoreKey: M.callWhen().returns(M.record()), makeChildNode: M.call(M.string()) @@ -107,18 +109,17 @@ harden(assertPathSegment); * Must match the switch in vstorage.go using `vstorageMessage` type * * @typedef { 'get' | 'getStoreKey' | 'has' | 'children' | 'entries' | 'values' |'size' } StorageGetByPathMessageMethod + * @typedef { 'delete' } StorageMultiPathMessageMethod * @typedef { 'set' | 'setWithoutNotify' | 'append' } StorageUpdateEntriesMessageMethod - * @typedef {StorageGetByPathMessageMethod | StorageUpdateEntriesMessageMethod } StorageMessageMethod + * @typedef { StorageGetByPathMessageMethod | StorageMultiPathMessageMethod | StorageUpdateEntriesMessageMethod } StorageMessageMethod * @typedef { [path: string] } StorageGetByPathMessageArgs + * @typedef { [...paths: string[]] } StorageMultiPathMessageArgs * @typedef { [path: string, value?: string | null] } StorageEntry * @typedef { StorageEntry[] } StorageUpdateEntriesMessageArgs - * @typedef {{ - * method: StorageGetByPathMessageMethod; - * args: StorageGetByPathMessageArgs; - * } | { - * method: StorageUpdateEntriesMessageMethod; - * args: StorageUpdateEntriesMessageArgs; - * }} StorageMessage + * @typedef {{ method: StorageGetByPathMessageMethod, args: StorageGetByPathMessageArgs }} StorageGetByPathMessage + * @typedef {{ method: StorageMultiPathMessageMethod, args: StorageMultiPathMessageArgs }} StorageMultiPathMessage + * @typedef {{ method: StorageUpdateEntriesMessageMethod, args: StorageUpdateEntriesMessageArgs }} StorageUpdateEntriesMessage + * @typedef { StorageGetByPathMessage | StorageMultiPathMessage | StorageUpdateEntriesMessage } StorageMessage */ /** @@ -195,6 +196,11 @@ export const prepareChainStorageNode = zone => { args: [entry], }); }, + /** @type {() => Promise} */ + async delete() { + const { path, messenger } = this.state; + await cb.callE(messenger, { method: 'delete', args: [path] }); + }, // Possible extensions: // * getValue() // * getChildNames() and/or makeChildNodes() diff --git a/packages/internal/src/storage-test-utils.js b/packages/internal/src/storage-test-utils.js index c17e6d0a12f..1756f7b2088 100644 --- a/packages/internal/src/storage-test-utils.js +++ b/packages/internal/src/storage-test-utils.js @@ -182,6 +182,15 @@ export const makeFakeStorageKit = (rootPath, rootOptions) => { } break; } + case 'delete': { + trace('toStorage delete', message); + /** @type {string[]} */ + const keys = message.args; + for (const key of keys) { + data.delete(key); + } + break; + } case 'size': // Intentionally incorrect because it counts non-child descendants, // but nevertheless supports a "has children" test. diff --git a/packages/internal/test/test-storage-test-utils.js b/packages/internal/test/test-storage-test-utils.js index 38021821f9f..ad59d30a2c0 100644 --- a/packages/internal/test/test-storage-test-utils.js +++ b/packages/internal/test/test-storage-test-utils.js @@ -189,6 +189,25 @@ test('makeFakeStorageKit', async t => { [{ method: 'set', args: [[childPath]] }], 'child setValue message', ); + + await childNode.delete(); + t.deepEqual( + messages.slice(-1), + [{ method: 'delete', args: [childPath] }], + 'child delete message', + ); + await deepNode.delete(); + t.deepEqual( + messages.slice(-1), + [{ method: 'delete', args: [deepPath] }], + 'granchild delete message', + ); + await childNode.delete(); + t.deepEqual( + messages.slice(-1), + [{ method: 'delete', args: [childPath] }], + 'child delete message', + ); }); test('makeFakeStorageKit sequence data', async t => { @@ -262,6 +281,19 @@ test('makeFakeStorageKit sequence data', async t => { [{ method: 'append', args: [[deepPath, 'qux']] }], 'manual-sequence grandchild setValue message', ); + + await childNode.delete(); + t.deepEqual( + messages.slice(-1), + [{ method: 'delete', args: [childPath] }], + 'child delete message', + ); + await deepNode.delete(); + t.deepEqual( + messages.slice(-1), + [{ method: 'delete', args: [deepPath] }], + 'granchild delete message', + ); }); const testUnmarshaller = test.macro((t, format) => { From 86a74d8b85a4ec633a31694c26de157a352be864 Mon Sep 17 00:00:00 2001 From: Richard Gibson Date: Tue, 18 Jul 2023 19:18:43 -0400 Subject: [PATCH 2/4] test(cosmos): Add more vstorage tests --- golang/cosmos/x/vstorage/vstorage_test.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/golang/cosmos/x/vstorage/vstorage_test.go b/golang/cosmos/x/vstorage/vstorage_test.go index 4b21fe01520..08988ac487c 100644 --- a/golang/cosmos/x/vstorage/vstorage_test.go +++ b/golang/cosmos/x/vstorage/vstorage_test.go @@ -88,7 +88,8 @@ func TestGetAndHas(t *testing.T) { {label: "empty non-terminal", args: []interface{}{"top.empty-non-terminal"}, want: `null`}, {label: "no entry", args: []interface{}{"nosuchpath"}, want: `null`}, {label: "empty args", args: []interface{}{}, errContains: ptr(`missing`)}, - {label: "non-string arg", args: []interface{}{42}, errContains: ptr(`json`)}, + {label: "number arg", args: []interface{}{42}, errContains: ptr(`json`)}, + {label: "null arg", args: []interface{}{nil}, errContains: ptr(`null`)}, {label: "extra args", args: []interface{}{"foo", "bar"}, errContains: ptr(`extra`)}, } for _, desc := range cases { @@ -159,12 +160,18 @@ func doTestSetAndDelete(t *testing.T, setMethod string, expectNotify bool) { {label: "restorations", args: []interface{}{[]string{"baz.b", "quux"}, []string{"baz.a", "quux"}}, }, - {label: "non-string path", + {label: "number path", // TODO: Fully validate input before making changes // args: []interface{}{[]string{"foo", "X"}, []interface{}{42, "new"}}, args: []interface{}{[]interface{}{42, "new"}}, errContains: ptr("path"), }, + {label: "null path", + // TODO: Fully validate input before making changes + // args: []interface{}{[]string{"foo", "Y"}, []interface{}{nil, "new"}}, + args: []interface{}{[]interface{}{nil, "new"}}, + errContains: ptr("path"), + }, {label: "non-string value", // TODO: Fully validate input before making changes // args: []interface{}{[]string{"foo", "X"}, []interface{}{"foo", true}}, From ce12eca496998d0180192a30331cf4426872ba8d Mon Sep 17 00:00:00 2001 From: Richard Gibson Date: Tue, 18 Jul 2023 19:21:25 -0400 Subject: [PATCH 3/4] fix(cosmos): Prevent promotion of `null` arguments to `""` in vstorage handlers --- golang/cosmos/x/vstorage/vstorage.go | 46 ++++++++++++---------------- 1 file changed, 19 insertions(+), 27 deletions(-) diff --git a/golang/cosmos/x/vstorage/vstorage.go b/golang/cosmos/x/vstorage/vstorage.go index e82aa966c3a..f497a9cf4df 100644 --- a/golang/cosmos/x/vstorage/vstorage.go +++ b/golang/cosmos/x/vstorage/vstorage.go @@ -41,14 +41,13 @@ func unmarshalString(jsonText json.RawMessage) (string, error) { return *str, nil } -func unmarshalSinglePathFromArgs(args []json.RawMessage, path *string) error { +func unmarshalSinglePathFromArgs(args []json.RawMessage) (string, error) { if len(args) == 0 { - return fmt.Errorf("missing 'path' argument") + return "", fmt.Errorf("missing 'path' argument") + } else if len(args) != 1 { + return "", fmt.Errorf("extra arguments after 'path'") } - if len(args) != 1 { - return fmt.Errorf("extra arguments after 'path'") - } - return json.Unmarshal(args[0], path) + return unmarshalString(args[0]) } func unmarshalPathsFromArgs(args []json.RawMessage) ([]string, error) { @@ -157,10 +156,9 @@ func (sh vstorageHandler) Receive(cctx *vm.ControllerContext, str string) (ret s case "get": // Note that "get" does not (currently) unwrap a StreamCell. - var path string - err = unmarshalSinglePathFromArgs(msg.Args, &path) + path, err := unmarshalSinglePathFromArgs(msg.Args) if err != nil { - return + return "", err } entry := keeper.GetEntry(cctx.Context, path) @@ -174,10 +172,9 @@ func (sh vstorageHandler) Receive(cctx *vm.ControllerContext, str string) (ret s return string(bz), nil case "getStoreKey": - var path string - err = unmarshalSinglePathFromArgs(msg.Args, &path) + path, err := unmarshalSinglePathFromArgs(msg.Args) if err != nil { - return + return "", err } value := vstorageStoreKey{ StoreName: keeper.GetStoreName(), @@ -192,10 +189,9 @@ func (sh vstorageHandler) Receive(cctx *vm.ControllerContext, str string) (ret s return string(bz), nil case "has": - var path string - err = unmarshalSinglePathFromArgs(msg.Args, &path) + path, err := unmarshalSinglePathFromArgs(msg.Args) if err != nil { - return + return "", err } value := keeper.HasStorage(cctx.Context, path) if !value { @@ -205,10 +201,9 @@ func (sh vstorageHandler) Receive(cctx *vm.ControllerContext, str string) (ret s // TODO: "keys" is deprecated case "children", "keys": - var path string - err = unmarshalSinglePathFromArgs(msg.Args, &path) + path, err := unmarshalSinglePathFromArgs(msg.Args) if err != nil { - return + return "", err } children := keeper.GetChildren(cctx.Context, path) if children.Children == nil { @@ -221,10 +216,9 @@ func (sh vstorageHandler) Receive(cctx *vm.ControllerContext, str string) (ret s return string(bytes), nil case "entries": - var path string - err = unmarshalSinglePathFromArgs(msg.Args, &path) + path, err := unmarshalSinglePathFromArgs(msg.Args) if err != nil { - return + return "", err } children := keeper.GetChildren(cctx.Context, path) entries := make([][]interface{}, len(children.Children)) @@ -243,10 +237,9 @@ func (sh vstorageHandler) Receive(cctx *vm.ControllerContext, str string) (ret s return string(bytes), nil case "values": - var path string - err = unmarshalSinglePathFromArgs(msg.Args, &path) + path, err := unmarshalSinglePathFromArgs(msg.Args) if err != nil { - return + return "", err } children := keeper.GetChildren(cctx.Context, path) vals := make([]string, len(children.Children)) @@ -260,10 +253,9 @@ func (sh vstorageHandler) Receive(cctx *vm.ControllerContext, str string) (ret s return string(bytes), nil case "size": - var path string - err = unmarshalSinglePathFromArgs(msg.Args, &path) + path, err := unmarshalSinglePathFromArgs(msg.Args) if err != nil { - return + return "", err } children := keeper.GetChildren(cctx.Context, path) if children.Children == nil { From 5bd85dd03c236a1ea3509f26fb9e39afee4d4527 Mon Sep 17 00:00:00 2001 From: Richard Gibson Date: Tue, 18 Jul 2023 22:14:39 -0400 Subject: [PATCH 4/4] chore: Fix lint issues --- packages/notifier/src/storesub.js | 5 +++-- packages/notifier/src/types-ambient.js | 6 +----- packages/notifier/tools/testSupports.js | 3 +++ 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/notifier/src/storesub.js b/packages/notifier/src/storesub.js index 1068fa3379a..5dcfd3e2781 100644 --- a/packages/notifier/src/storesub.js +++ b/packages/notifier/src/storesub.js @@ -140,10 +140,11 @@ export const makeStoredSubscription = ( /** @type {StoredSubscription} */ const storesub = Far('StoredSubscription', { - // @ts-expect-error getStoreKey type does not have `subscription` getStoreKey: async () => { if (!storageNode) { - return harden({ subscription }); + return /** @type {Awaited['getStoreKey']>>} */ ( + harden({ subscription }) + ); } const storeKey = await E(storageNode).getStoreKey(); return harden({ ...storeKey, subscription }); diff --git a/packages/notifier/src/types-ambient.js b/packages/notifier/src/types-ambient.js index c0e7c40db34..8d6a9c36722 100644 --- a/packages/notifier/src/types-ambient.js +++ b/packages/notifier/src/types-ambient.js @@ -317,11 +317,7 @@ * identifies children by restricted ASCII name and is associated with arbitrary * string-valued data for each node, defaulting to the empty string. * - * @typedef {object} StorageNode - * @property {(data: string) => Promise} setValue publishes some data (append to the node) - * @property {() => string} getPath the chain storage path at which the node was constructed - * @property {() => Promise} getStoreKey DEPRECATED use getPath - * @property {(subPath: string, options?: {sequence?: boolean}) => StorageNode} makeChildNode + * @typedef {import('@agoric/internal/src/lib-chainStorage').StorageNode} StorageNode */ /** diff --git a/packages/notifier/tools/testSupports.js b/packages/notifier/tools/testSupports.js index f1eceb40db7..4adfc808edd 100644 --- a/packages/notifier/tools/testSupports.js +++ b/packages/notifier/tools/testSupports.js @@ -28,6 +28,9 @@ export const makeFakeStorage = (path, publication) => { publication.updateState(value); } }, + delete: async () => { + throw Error('not implemented'); + }, makeChildNode: () => storage, countSetValueCalls: () => setValueCalls, });