feat: support offer signing with keplr #28

samsiegart · 2023-08-15T07:14:03Z

fixes #29

I'd like to follow up with documentation/code examples in the README for reading vstorage, reading users' purse balances, and making offers.

samsiegart · 2023-08-15T07:15:06Z

packages/web-components/src/wallet-connection/makeInteractiveSigner.js

This file was taken from wallet-app and trimmed down a bit to remove the non-spend-action stuff.

samsiegart · 2023-08-15T07:49:01Z

We can see from https://github.com/Agoric/ui-kit/actions/runs/5864651371/job/15900068743?pr=28 that the test now requires ses because of the dependencies on casting and notifiers. After adding ses to the test environment, I now get the below error. Maybe it's possible to migrate these tests to Ava, but vitest is not cooperating for some reason in this context.

yarn run v1.22.19
$ vitest

 DEV  v0.32.0 /home/samsiegart/ui-kit/packages/web-components

FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal.
 1: 0xb6e500 node::Abort() [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
 2: 0xa7e53e node::FatalError(char const*, char const*) [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
 3: 0xd47c3a v8::Utils::ReportApiFailure(char const*, char const*) [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
 4: 0xb70575  [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]

 5: 0xb7093e node::errors::TryCatchScope::~TryCatchScope() [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
 6: 0xb70ec8 node::errors::TriggerUncaughtException(v8::Isolate*, v8::Local<v8::Value>, v8::Local<v8::Message>, bool) [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Errors ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

Vitest caught 1 unhandled error during the test run.
This might cause false positive tests. Resolve unhandled errors to make sure your tests are not affected.
 7: 0xb71540 node::errors::PerIsolateMessageListener(v8::Local<v8::Message>, v8::Local<v8::Value>) [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
 8: 0xeb3610 v8::internal::MessageHandler::ReportMessageNoExceptions(v8::internal::Isolate*, v8::internal::MessageLocation const*, v8::internal::Handle<v8::internal::Object>, v8::Local<v8::Value>) [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
 9: 0xeb3821 v8::internal::MessageHandler::ReportMessage(v8::internal::Isolate*, v8::internal::MessageLocation const*, v8::internal::Handle<v8::internal::JSMessageObject>) [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Error ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
TypeError: Cannot assign to read only property 'random' of object '[object Math]'
 ❯ process.emit node:events:525:35
10: 0xea2661 v8::internal::Isolate::ReportPendingMessages() [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

11: 0xe88858  [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
 Test Files  no tests
      Tests  no tests
     Errors  1 error
12: 0xe8962f v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
   Start at  00:43:31
   Duration  268ms (transform 31ms, setup 0ms, collect 0ms, tests 0ms, environment 0ms, prepare 0ms)
13: 0xd63c63 v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]

14: 0xaacd6f node::InternalCallbackScope::Close() [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]

 FAIL  Tests failed. Watching for file changes...
       press h to show help, press q to quit
15: 0xaacde1 node::InternalCallbackScope::~InternalCallbackScope() [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
16: 0xb734f7 node::fs::FileHandle::CloseReq::Resolve() [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
17: 0xb758c4  [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
18: 0xb6b000 node::MakeLibuvRequestCallback<uv_fs_s, void (*)(uv_fs_s*)>::Wrapper(uv_fs_s*) [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
19: 0x16482ad  [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
20: 0x164ca86  [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
21: 0x165f1c4  [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
22: 0x164d3d8 uv_run [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
23: 0xaad9e5 node::SpinEventLoop(node::Environment*) [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
24: 0xc35e0f node::worker::Worker::Run() [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
25: 0xc363c8  [/home/samsiegart/.nvm/versions/node/v18.12.1/bin/node]
26: 0x7f3ba0f3fb43  [/lib/x86_64-linux-gnu/libc.so.6]
27: 0x7f3ba0fd1a00  [/lib/x86_64-linux-gnu/libc.so.6]
Aborted
error Command failed with exit code 134.

turadg · 2023-08-15T12:01:54Z

vitest is not cooperating for some reason in this context

vitest-dev/vitest#3527 has the fix. Released in 0.32.3. This repo is on 0.32.0.

packages/web-components/package.json

turadg · 2023-08-15T12:17:18Z

patches/@agoric+smart-wallet+0.5.3.patch

+      */
+     fromBoard: (slot, iface) => {
+-      isDefaultBoardId(slot) || Fail`bad board slot ${q(slot)}`;
+      isDefaultBoardId(slot) || slot === null || Fail`bad board slot ${q(slot)}`;


how do we stop patching this?

This app only needs the board marshaller, not the whole import context the smart-wallet itself needs.

Would this work? https://github.com/Agoric/agoric-sdk/blob/c6d8a278ad7d7c0ad758f57c7e4db858d6a67e53/packages/internal/src/marshal.js#L32-L41

Yes, I think that will work, but while working on productizing the bidding CLI, I realized that relying on remotables to know their slot/boadID is awkward.

Also, the BoardRemote${nonalleged} idiom is over-kill. IMO it's better, conceptually, to just use ${nonalleged} as the debug name. But more importantly: users pay by the byte when they submit offers.

The way I've been teaching it lately is to use a typical translation table, parameterized by what to do when you get a cache miss...

https://github.com/Agoric/documentation/blob/daf6472eaa257bb28c78341f521a87fd684bcd99/snippets/test-marshal.js#L97-L121

And the board slotting marshaller synthesizes a remotable when slotToVal has a cache miss:

https://github.com/Agoric/documentation/blob/daf6472eaa257bb28c78341f521a87fd684bcd99/snippets/test-marshal.js#L319C1-L326C81

Okay I got something like Dan's example working

yarn.lock

package.json

dckc · 2023-08-15T14:53:54Z

packages/web-components/src/wallet-connection/makeInteractiveSigner.js

+ * @param {Keplr} keplr
+ * @param {typeof import('@cosmjs/stargate').SigningStargateClient.connectWithSigner} connectWithSigner


Glad to see the use of explicit authority is still here. :)

turadg · 2023-08-15T19:38:57Z

packages/rpc/package.json

@@ -29,6 +30,6 @@
    "postinstall-postinstall": "^2.1.0",
    "prettier": "^2.7.1",
    "typescript": "^5.1.3",
-    "vitest": "^0.32.0"
+    "vitest": "0.34.1"


Suggested change

"vitest": "0.34.1"

"vitest": "^0.34.1"

Done, but what is the benefit of this? Doesn't this make our build less deterministic?

turadg · 2023-08-15T19:39:05Z

packages/rpc/package.json

@@ -20,6 +20,7 @@
    "vite-tsconfig-paths": "^4.2.0"
  },
  "devDependencies": {
+    "@endo/marshal": "0.8.8",


Suggested change

"@endo/marshal": "0.8.8",

"@endo/marshal": "^0.8.8",

turadg · 2023-08-15T19:39:51Z

packages/rpc/src/chainStorageWatcher.ts

+/* eslint-disable import/no-extraneous-dependencies */
+import { makeMarshal } from 'marshal';


We should really avoid undeclared dependencies. I think this is just as simple as:

Suggested change

/* eslint-disable import/no-extraneous-dependencies */

import { makeMarshal } from 'marshal';

import { makeMarshal } from './marshal';

Interesting... done

turadg · 2023-08-15T19:41:00Z

packages/rpc/src/chainStorageWatcher.ts

  onError?: (e: Error) => void,
+  marshal = makeMarshal(),


consider calling it "marshaller" since "marshal" is a verb and this is an object that holds functions/verbs

Done, but then shouldn't the endo export be named makeMarshaller instead also?

turadg · 2023-08-15T19:41:44Z

packages/rpc/src/marshal.ts

@@ -0,0 +1,40 @@
+/* eslint-disable import/no-extraneous-dependencies */


isn't it declared?

Suggested change

/* eslint-disable import/no-extraneous-dependencies */

/* eslint-disable import/no-extraneous-dependencies */

'@endo/marshal' should be listed in the project's dependencies, not devDependencies. I didn't think it was worth putting in dependencies, but done

turadg · 2023-08-15T19:44:58Z

packages/rpc/src/marshal.ts

+export const makeMarshal = () =>
+  endoMakeMarshal(convertValToSlot, convertSlotToVal, {


if someone is familiar with Endo, makeMarshal has a learned meaning.

consider exporting makeTranslationMarshaller or makeClientMarshaller and allowing Endo's to keep its name.

turadg · 2023-08-15T19:45:27Z

packages/rpc/test/chainStorageWatcher.test.ts

+        unserialize: unmarshal,
+        serialize: marshal,


we could drop these. they're deprecated.

Needed to satisfy the type

turadg · 2023-08-15T19:47:11Z

packages/web-components/src/wallet-connection/walletConnection.js

+    );
+
+    try {
+      // eslint-disable-next-line @jessie.js/safe-await-separator


try an await null before the this try{}. the lint rule shouldn't complain

Done, I just wasn't sure that was any better.

turadg · 2023-08-15T19:47:56Z

packages/web-components/test/walletConnection.test.js

@@ -60,4 +69,32 @@ describe('makeAgoricWalletConnection', () => {
      Errors.noSmartWallet,
    );
  });
+
+  it('submits a spend action', async () => {


remark: yay tests!

samsiegart · 2023-08-16T17:09:23Z

packages/rpc/src/chainStorageWatcher.ts

@@ -1,9 +1,9 @@
 /* eslint-disable no-use-before-define */
 /* eslint-disable import/extensions */
-import type { FromCapData } from '@endo/marshal';
-import type { UpdateHandler } from './types';
+import { makeClientMarshaller } from './marshal';


See "marshal.ts" for an example that should work with null slots.

dckc · 2023-08-16T17:12:47Z

packages/rpc/src/marshal.ts

+  };
+
+  const convertSlotToVal = (slot: unknown, iface: string | undefined) => {
+    if (slotToVal.has(slot)) return slotToVal.get(slot);


hm. a slot of null should probably be treated as a cache miss here.

Do you mean we should just do something like:

if (slot === null) { return Far('null', {}) }

I can't see clearly why it's bad for all null slots to resolve to the same presence

if on-chain code uses the read-only marshaller to marshal distinct unpublished remotables X and Y, they both come across with null slots. This current code unmarshalls them to one object. That could cause a client to think that 2 different brands are the same or all sorts of other stuff... if it's not exploitable now, we'll have to carefully check every change to what our code (ALL of our code) does to make sure it continues to be not exploitable.

The down side of treating null as a cache-miss is that two references to the same X get unmarshalled as distinct objects. Given that null is supposed to not communicate anything about the identity of what was in that slot, that seems fail-safe, to me. It's not without some risk, though. Do we know where the null is coming from? It's supposed to indicate a bug.

Do you mean we should just do something like:

if (slot === null) { return Far('null', {}) }

something like that, yes. But specifically: it should return makeVal(slot, iface);

Done. Yea, I was thinking along the lines of "null slot means null value on-chain", so thought it was okay if they were treated as null === null, but unpublished values that are actually different makes sense.

Do we know where the null is coming from? It's supposed to indicate a bug. - @dckc

I observe(d) null slots in mainnet but not on devnet, so this may already be addressed.

When i query published.vaultFactory.managers.manager0.quotes and pass the result into importContext.fromBoard.fromCapData(), it throws with Error: bad board slot null.

The offending data blob:

Object <[Object: null prototype] {}> { body: '#{"quoteAmount":{"brand":"$0.Alleged: quote brand","value":[{"amountIn":{"brand":"$1.Alleged: ATOM brand","value":"+1000000"},"amountOut":{"brand":"$2.Alleged: IST brand","value":"+6525815"},"timer":"$3.Alleged: timerService","timestamp":{"absValue":"+1694631252","timerBrand":"$4.Alleged: timerBrand"}}]},"quotePayment":"$5.Alleged: quote payment"}', slots: [ null, 'board05557', 'board0257', 'board05674', 'board0425', null ] }

Happy to create an issue in the main repo if needed.

It's expected for vstorage to have null slots because not all objects referenced are necessarily public. For instance, the final slot in the quote example is for a quote payment, which must not be published.

turadg

LGTM. But I'm not certain of the point DC is making about cache miss.

samsiegart commented Aug 15, 2023

View reviewed changes

samsiegart force-pushed the keplr-signing branch from bcd1575 to cdad864 Compare August 15, 2023 07:19

samsiegart added 2 commits August 15, 2023 00:19

feat: support offer signing with keplr

cdad864

test: remove broken test

604a129

test: fix chainStorageWatcher test

bc2b2e5

samsiegart requested a review from turadg August 15, 2023 08:05

turadg reviewed Aug 15, 2023

View reviewed changes

dckc reviewed Aug 15, 2023

View reviewed changes

test: revive walletConnection test

3a8b5cc

samsiegart requested review from turadg and dckc August 15, 2023 19:29

samsiegart force-pushed the keplr-signing branch from 076a3e9 to baef932 Compare August 15, 2023 19:35

refactor: dont depend on patch for marshal

baef932

turadg reviewed Aug 15, 2023

View reviewed changes

chore: lint and naming

1942589

samsiegart requested a review from turadg August 15, 2023 22:51

samsiegart commented Aug 16, 2023

View reviewed changes

dckc reviewed Aug 16, 2023

View reviewed changes

turadg approved these changes Aug 16, 2023

View reviewed changes

fix: dont resolve null slots with same val

a92a05a

samsiegart enabled auto-merge (rebase) August 16, 2023 22:29

Merge branch 'main' into keplr-signing

87a68d8

samsiegart merged commit 7cf64bb into main Aug 16, 2023
1 check passed

samsiegart deleted the keplr-signing branch August 16, 2023 22:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support offer signing with keplr #28

feat: support offer signing with keplr #28

samsiegart commented Aug 15, 2023 •

edited

Loading

samsiegart Aug 15, 2023

samsiegart commented Aug 15, 2023 •

edited

Loading

turadg commented Aug 15, 2023 •

edited

Loading

turadg Aug 15, 2023

dckc Aug 15, 2023

samsiegart Aug 15, 2023

dckc Aug 15, 2023

turadg Aug 15, 2023

samsiegart Aug 15, 2023

turadg Aug 15, 2023

samsiegart Aug 15, 2023

turadg Aug 15, 2023

samsiegart Aug 15, 2023

turadg Aug 15, 2023

samsiegart Aug 15, 2023

turadg Aug 15, 2023

samsiegart Aug 15, 2023

turadg Aug 15, 2023

samsiegart Aug 15, 2023

turadg Aug 15, 2023

samsiegart Aug 15, 2023

turadg Aug 15, 2023

samsiegart Aug 15, 2023

turadg Aug 15, 2023

samsiegart Aug 16, 2023

dckc Aug 16, 2023

samsiegart Aug 16, 2023

samsiegart Aug 16, 2023

dckc Aug 16, 2023

dckc Aug 16, 2023

samsiegart Aug 16, 2023

0xpatrickdev Sep 13, 2023 •

edited

Loading

turadg Sep 13, 2023

turadg left a comment

		* @param {Keplr} keplr
		* @param {typeof import('@cosmjs/stargate').SigningStargateClient.connectWithSigner} connectWithSigner

		/* eslint-disable import/no-extraneous-dependencies */
		import { makeMarshal } from 'marshal';

	/* eslint-disable import/no-extraneous-dependencies */
	import { makeMarshal } from 'marshal';
	import { makeMarshal } from './marshal';

		@@ -0,0 +1,40 @@
		/* eslint-disable import/no-extraneous-dependencies */

		export const makeMarshal = () =>
		endoMakeMarshal(convertValToSlot, convertSlotToVal, {

feat: support offer signing with keplr #28

feat: support offer signing with keplr #28

Conversation

samsiegart commented Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

samsiegart commented Aug 15, 2023 • edited Loading

turadg commented Aug 15, 2023 • 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

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

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

0xpatrickdev Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

samsiegart commented Aug 15, 2023 •

edited

Loading

samsiegart commented Aug 15, 2023 •

edited

Loading

turadg commented Aug 15, 2023 •

edited

Loading

0xpatrickdev Sep 13, 2023 •

edited

Loading