feat(wallet/ui): interactive, background signing #5877

dckc · 2022-08-02T16:01:59Z

closes: #4406
refs: #3628; obsoletes #5761

Description

makeInteractiveSigner supports signing offers as in the initial smart wallet swap demo. (#5354)

It also supports authorizing a background signer which we plan to use in other user stories.
In particular, it uses authz. (It should use feegrant too, but see #5825 as well as cosmos/cosmjs#1155 )

Security Considerations

The secret key to the background signer is stored in localStorage in the clear, protected only by the same-origin policy; this risk is moderate, given:

the authz grant is limited to low-privilege MsgWalletAction messages
the grant is limited to 4hrs
the grant is limited to spending 0.25IST

Documentation Considerations

End-user documentation for this feature is in order, not to mention completed UI support.

Testing Considerations

Testing so far is manual. (integration test built using a temporary button removed on request)

dckc · 2022-08-03T15:33:02Z

naming idea: makeNonspendingKey

brainstorming with @turadg

without ledger - toward keplr/ledger demo: copy SuggestChain.js - sign simple msg/doc with keplr/ledger (WIP) - ISSUE: started with cosmjs API integration but couldn't get it to work without skipping it - chore(demo-signing): figured out offlignSigner.signAmino() API no longer skipping all the way to keplr.signAmino() - feat(demo-signing): sign WALLET_ACTION with keplr, ledger - send signed transaction to chain (WIP) - can connect to chain and query for sequence - aminoTypes seem to work - TODO: generate protobuf codec for MsgWalletAction - pass connectWithSigner authority explicitly - integrate MsgWalletAction proto stub via registry - MsgWalletAction.owner is bytes, not string provide enough gas avoid: Error: Broadcasting transaction failed with code 11 (codespace: sdk). Log: out of gas in location: ReadFlat; gasWanted: 250, gasUsed: 1000: out of gas at SigningStargateClient.broadcastTx (stargateclient.ts:413:9) at async HTMLButtonElement.<anonymous> (keplr-ledger-demo.js:215:16)

@type

- workaround: cosmjs lacks support for fee-account in MsgExec - misc static types addressed: Error: Broadcasting transaction failed with code 2 (codespace: sdk). Log: no concrete type registered for type URL /agoric.swingset.MsgWalletAction against interface *authz.Authorization: tx parse error at SigningStargateClient.broadcastTx (stargateclient.ts:413:9) at async Object.authorizeMessagingKey (keplr-ledger-demo.js:183:18) basic fee grant to messaging key example result: agoric-sdk/packages/cosmic-swingset$ ~/go/bin/agd query tx E0EDE9B200AB0B9FF33DA288BC9E455C228854D76D5AD12C80EA5138C6706D06 ... timestamp: "2022-07-20T03:07:21Z" tx: '@type': /cosmos.tx.v1beta1.Tx auth_info: fee: amount: - amount: "0" denom: uist gas_limit: "100000" granter: "" payer: "" signer_infos: - mode_info: single: mode: SIGN_MODE_DIRECT public_key: '@type': /cosmos.crypto.secp256k1.PubKey key: AggWzZHxF8d7Quy/N9UUGWwU05pIEJKNFGFPpfx01Ika sequence: "4" body: extension_options: [] memo: "" messages: - '@type': /cosmos.authz.v1beta1.MsgGrant grant: authorization: '@type': /cosmos.authz.v1beta1.GenericAuthorization msg: /agoric.swingset.MsgWalletAction expiration: "2022-07-20T07:07:22Z" grantee: agoric12ufmm960ynt0svr8c7gwy9he909z34w73fkue8 granter: agoric1uh4ynz9h3ea9vc80tl30s4ykmcyqwfl0d7zelw - '@type': /cosmos.feegrant.v1beta1.MsgGrantAllowance allowance: '@type': /cosmos.feegrant.v1beta1.BasicAllowance expiration: "2022-07-20T07:07:22Z" spend_limit: - amount: "250000" denom: uist grantee: agoric12ufmm960ynt0svr8c7gwy9he909z34w73fkue8 granter: agoric1uh4ynz9h3ea9vc80tl30s4ykmcyqwfl0d7zelw non_critical_extension_options: [] timeout_height: "0" signatures: - JTO/ouDtWgTB8fO8ngHceQk+AxIup8tWClaKEFCOqiE1N5gC2TEVN2CiKch2tmE6/Pj2NXJH9DoGFEpz5WBB7A== txhash: E0EDE9B200AB0B9FF33DA288BC9E455C228854D76D5AD12C80EA5138C6706D06 feat(demo-signing): use ephemeral messaging key to do actions (WIP) struggling to get it to work. currently gives: "failed to execute message; message index: 0: authorization not found: unauthorized" despite: $ agd query authz grants agoric1uh4ynz9h3ea9vc80tl30s4ykmcyqwfl0d7zelw agoric1prh40kcgtx4uxd870aye5swmqzdrfu4ggp5pfm grants: - authorization: '@type': /cosmos.authz.v1beta1.GenericAuthorization msg: /agoric.swingset.MsgWalletAction expiration: "2022-07-20T08:42:41Z"

grant fails thusly: aminotypes.ts:38 Uncaught (in promise) Error: The message type '/cosmos.authz.v1beta1.MsgGrant' cannot be signed using the Amino JSON sign mode because this is not supported by chain. at AminoTypes.toAmino (aminotypes.ts:38:13) at signingstargateclient.ts:351:56 at Array.map (<anonymous>) at SigningStargateClient.signAmino (signingstargateclient.ts:351:27) at async SigningStargateClient.signAndBroadcast (signingstargateclient.ts:295:19) at async Object.authorizeLocalKey (keyManagement.js:350:18) and submit offer thusly: Uncaught (in promise) Error: Broadcasting transaction failed with code 4 (codespace: sdk). Log: signature verification failed; please verify account number (14), sequence (0) and chain-id (agoric): unauthorized at SigningStargateClient.broadcastTx (stargateclient.ts:413:9) at async Object.submitSpendAction (keyManagement.js:381:18)

- allow caller to pass authority explicitly

- include interactiveSigner, backgroundSigner in state.keplrConnection - punt window.cosmJS, whatever that was - use tryKeplrConnect in ChainConnector - move ambient authority to the edges: Provider.jsx - clean up SuggestChain

samsiegart

I don't have a grasp of all the stuff in keyManagement.js but the rest looks good to me assuming we're getting keplr to actually pop up with an approval window. Will defer approval to @michaelfig for the rest.

samsiegart · 2022-08-04T22:38:52Z

packages/wallet/ui/src/components/Requests.jsx

  return (
    <div className="Requests">
      {requests.length ? (
        requests.map(Item)
      ) : (
        <div className="Empty">
+          <button type="button" onClick={signOffer}>


I think it's okay to not include this button in the merge, we have the code in other branches to reference

ok; it's gone

dckc · 2022-08-05T15:07:35Z

chatting with @michaelfig

the grant is limited to 4hrs

he suggests aligning with a browser localStorage norm.

Also, maybe @rowgraus should weigh in? should that be a week?

michaelfig

Given fixes, this looks good to merge!

michaelfig · 2022-08-05T15:10:52Z

packages/wallet/ui/package.json

@@ -8,6 +8,7 @@
  "type": "module",
  "devDependencies": {
    "@agoric/assert": "^0.4.0",
+    "@agoric/cosmic-proto": "0.1.0",


Yaay! Thanks @kriskowal!

michaelfig · 2022-08-05T15:12:42Z

packages/wallet/ui/src/contexts/Provider.jsx

@@ -2,6 +2,8 @@
 import { observeIterator } from '@agoric/notifier';
 import { E } from '@endo/far';
 import { useEffect, useState, useReducer } from 'react';
+import { SigningStargateClient } from '@cosmjs/stargate';


Let's (later) introduce a powerful module that closes over these dependencies and is imported by Provider.jsx to keep Provider.jsx simpler.

It's fine for now.

michaelfig · 2022-08-05T15:14:53Z

packages/wallet/ui/src/util/chainInfo.js

@@ -0,0 +1,26 @@
+// @ts-check
+
+/** @type {import('@keplr-wallet/types').Currency} */


Noice toipes.

packages/wallet/ui/src/util/keyManagement.js

michaelfig · 2022-08-05T15:23:29Z

packages/wallet/ui/src/util/keyManagement.js

+ *
+ * arbitrary; suffix chosen randomly to avoid collisions
+ */
+export const STORAGE_KEY = 'agoric.eis0Aigi';


Suggested change

export const STORAGE_KEY = 'agoric.eis0Aigi';

export const BROWSER_LOCAL_STORAGE_KEY = 'agoric.eis0Aigi';

michaelfig · 2022-08-05T15:26:11Z

packages/wallet/ui/src/util/keyManagement.js

+  const accounts = await wallet.getAccounts();
+  console.debug('device account(s):', accounts);
+
+  const [{ address }] = accounts;


Use the same ?. expression here as elsewhere and throw a nicer error?

On further consideration, elsewhere we were dealing with the keplr API, which is doing IO to come up with an address and hence may fail. In this case, we're getting the address from DirectSecp256k1Wallet, which is doing only integer calculations that don't have that sort of failure mode. Any failures would show up at the await wallet.getAccounts() above.

packages/wallet/ui/src/util/keyManagement.js

turadg

Some small changes requested.

turadg · 2022-08-05T15:50:27Z

packages/wallet/ui/src/util/keyManagement.js

+/**
+ * key for use in localStorage
+ *
+ * arbitrary; suffix chosen randomly to avoid collisions


I don't get this random string. Wouldn't it be better to be human readable as to what's kept inside? e.g. agoric.wallet.backgroundSignerKey.

It could be versioned to signal breaking changes, but I think if backwards compatibility breaks you'd just want to empty it out and start over.

it's not terribly well motivated... sort of a vague fear of colliding with... I don't know what.

Giving it some thought, I suppose it's scoped to our origin. So I just need to be sure I don't collide with any other code we write. But again... genpass -1 is kind of a quick-and-dirty way to be 99.99999% sure of that :)

The alternative seems to be some sort of central registry agreed by all (potential) contributors.

Or maybe agoric.wallet.backgroundSignerKey has as effectively as much entropy as genpass -1 and I shouldn't worry about collisions?

Or maybe agoric.wallet.backgroundSignerKey has as effectively as much entropy as genpass -1 and I shouldn't worry about collisions?

Yes, I believe that to be the case.

turadg · 2022-08-05T15:54:16Z

packages/wallet/ui/src/contexts/Provider.jsx

@@ -148,11 +154,36 @@ const Provider = ({ children }) => {
  );
  const [keplrConnection, setKeplrConnection] = useState(null);

+  /**
+   * NOTE: relies on ambient window.fetch, window.keplr, Random.getBytes


better to document this with code, which will provide feedback when the assumptions are violated. e.g. down below,

assert(fetch, 'Missing window.fetch'); assert(keplr, 'Missing window.keplr'); assert(getBytes, 'Missing Random.getBytes');

asserts for fetch and keplr make sense, but Missing Random.getBytes would be a static error.
Random.getBytes is still noteworthy because it is ambient authority, contrary to OCap Discipline.

turadg · 2022-08-05T15:57:24Z

packages/wallet/ui/src/contexts/Provider.jsx

@@ -202,7 +233,9 @@ const Provider = ({ children }) => {
    if (
      connectionConfig?.smartConnectionMethod === SmartConnectionMethod.KEPLR
    ) {
-      tryKeplrConnect();
+      tryKeplrConnect().catch(reason =>
+        console.error('tryKeplrConnect failed', reason),


we should have an error toast for failures like this. if we don't have a toast component yet, adding one is out of scope but should be scheduled work before launch. cc @samsiegart

Yea we have toasts but I think I need to do some work to make the toast service more robust and reusable across the app

I'm adding a TODO.

turadg · 2022-08-05T15:57:44Z

packages/wallet/ui/src/util/SuggestChain.js


 export const AGORIC_COIN_TYPE = 564;
 export const COSMOS_COIN_TYPE = 118;

-export async function suggestChain(networkConfig, caption = undefined) {
+const makeChainInfo = (networkConfig, rpcAddr, chainId, caption) => {


please type

turadg · 2022-08-05T16:01:46Z

packages/wallet/ui/src/util/SuggestChain.js

+export async function suggestChain(
+  networkConfig,
+  /** @type {string} */ caption = undefined,
+  io = {},
+) {


if caption can be undefined then it's not string. let's type the function signature instead of inline.

Suggested change

export async function suggestChain(

networkConfig,

/** @type {string} */ caption = undefined,

io = {},

) {

/** @type {{

* networkConfig: NetworkConfig

* caption?: string

* io?: Partial<IoPowers>

* }}

*/

export async function suggestChain(networkConfig,, caption, io = {}) {

Though second thought, why make io optional? Please make them required so this module does not depend anything ambient. Then you can remove /* global globalThis */.

Why indeed. It looks like we addressed the only call site in this regard.

turadg · 2022-08-05T16:08:10Z

packages/wallet/ui/src/util/keyManagement.js

+ * @param {typeof window.localStorage} io.localStorage
+ * @param {typeof import('@cosmjs/crypto').Random.getBytes} io.getBytes for key generation
+ */
+export const makeBackgroundSigner = async ({ localStorage, getBytes }) => {


getBytes without the Random namespace reads overly generic. please rename to something that conveys that it's generating random bytes. consider makeRandomBytes.

that could mean having to map the name at the callsite but I'd also suggest changing this from an option map to positional parameters. then the caller doesn't have to be concerned with the name in this context.

I'm pretty fond of named parameters for IO powers and have spread the convention pretty far by now :) IOU docs to institutionalize it, perhaps.

It's also my habit to inherit the names from whatever API they come from, but I'll grant that getBytes is pretty worthless in this context. Changing to io.csprng.

packages/wallet/ui/src/util/keyManagement.js

turadg · 2022-08-05T16:10:41Z

packages/wallet/ui/src/util/keyManagement.js

+   * For example, to check whether `delegateWalletAction()` is necessary.
+   *
+   * @param {string} granter address
+   * @param {import('@cosmjs/tendermint-rpc').Tendermint34Client} rpcClient


turadg · 2022-08-05T16:12:06Z

packages/wallet/ui/src/util/keyManagement.js

+    });
+    const decoded = result.grants.map(
+      g =>
+        g.authorization && GenericAuthorization.decode(g.authorization.value),


should decoded contain falsy? if not, filter before returning.

also if it wasn't supposed to, consider defining the return type of queryGrants as that would have highlighted the problem.

Co-authored-by: Michael FIG <michael+github@fig.org>

Co-authored-by: Turadg Aleahmad <turadg@agoric.com> Co-authored-by: Michael FIG <michael+github@fig.org>

Co-authored-by: Michael FIG <michael+github@fig.org>

turadg · 2022-08-06T17:53:57Z

packages/wallet/ui/src/util/keyManagement.js

+  },
+};
+
+// ack: justerest Sep 2019 https://stackoverflow.com/a/58110124/7963


a little ugly but I see it's necessary. might want to reference UNTIL https://github.com/microsoft/TypeScript/issues/16655 so we know when we can remove this

oh. JS Arrays seem to have flatMap now... i.e. map and filter in one...

dckc changed the base branch from master to cosmic-swingset-proto August 3, 2022 23:42

kriskowal force-pushed the cosmic-swingset-proto branch 2 times, most recently from 256f5d5 to a91d1ca Compare August 4, 2022 04:00

dckc force-pushed the dc-wallet-signing-prod branch from 2189f62 to eb8f575 Compare August 4, 2022 04:12

kriskowal force-pushed the cosmic-swingset-proto branch from a91d1ca to 03cd1b2 Compare August 4, 2022 04:14

dckc force-pushed the dc-wallet-signing-prod branch from eb8f575 to bd078d3 Compare August 4, 2022 05:09

Base automatically changed from cosmic-swingset-proto to master August 4, 2022 05:16

dckc force-pushed the dc-wallet-signing-prod branch from bd078d3 to 40b4902 Compare August 4, 2022 05:20

dckc added 18 commits August 4, 2022 00:22

build(demo-signing): cosmjs, parcel, ... for keplr/ledger demo

bf9094f

feat(demo-signing): query WalletAction grants

3937c52

chore(demo-signing): factor out keyManagement.js

d31c587

refactor(wallet/ui): SuggestChain: separate IO from compute

b975d2b

- allow caller to pass authority explicitly

build(wallet/ui): devDependencies += @agoric/cosmic-proto

0457ec1

chore(wallet/ui): use packaged protobuf stubs

5e218eb

chore(wallet/ui): move keyManagement.js from demo-signing

f61d8b9

build(wallet/ui): cosmjs, keplr dependencies

c479291

fix(wallet/ui): handle local-chain rpcAddr

bd9ae9d

feat(wallet/ui): sign offer-like JSON (WIP)

324000a

chore(demo-signing): prune

a89b753

refactor(wallet/ui): factor out chainInfo.js

ecacaa5

chore(wallet/ui): clean up logging in keyManagement.js

6b07a75

refactor(wallet/ui): keyManagement naming, docs

bf48bae

feat(wallet/ui): support WalletSpendAction

5aa2863

dckc force-pushed the dc-wallet-signing-prod branch from 40b4902 to 5aa2863 Compare August 4, 2022 05:22

chore(wallet/ui): sign offer-like JSON (cont.)

0acc07d

- include interactiveSigner, backgroundSigner in state.keplrConnection - punt window.cosmJS, whatever that was - use tryKeplrConnect in ChainConnector - move ambient authority to the edges: Provider.jsx - clean up SuggestChain

dckc changed the title ~~feat(wallet/ui): WalletAction, WalletSpendAction signing (WIP)~~ feat(wallet/ui): interactive, background signing Aug 4, 2022

dckc marked this pull request as ready for review August 4, 2022 22:08

dckc requested review from samsiegart, michaelfig and turadg August 4, 2022 22:08

samsiegart reviewed Aug 4, 2022

View reviewed changes

chore(wallet/ui): never mind signing test button

3eda014

michaelfig approved these changes Aug 5, 2022

View reviewed changes

turadg requested changes Aug 5, 2022

View reviewed changes

dckc and others added 8 commits August 6, 2022 10:53

fixup: freeze->harden

62c8097

Co-authored-by: Michael FIG <michael+github@fig.org>

fixup: logging misc

6ad718a

Co-authored-by: Turadg Aleahmad <turadg@agoric.com> Co-authored-by: Michael FIG <michael+github@fig.org>

fix(keyManagement): urun->uist

53b8985

Co-authored-by: Michael FIG <michael+github@fig.org>

refactor(wallet/ui): rename getBytes -> csprng

221d4bd

fixup! chore(wallet/ui): sign offer-like JSON (cont.)

9843488

fixup! chore(wallet/ui): sign offer-like JSON (cont.)

4b9250a

fixup! chore(demo-signing): factor out keyManagement.js

439945a

fixup! refactor(wallet/ui): keyManagement naming, docs

a256b53

dckc requested a review from turadg August 6, 2022 17:18

dckc added the automerge:squash Automatically squash merge label Aug 6, 2022

turadg approved these changes Aug 6, 2022

View reviewed changes

mergify bot and others added 2 commits August 6, 2022 17:54

Merge branch 'master' into dc-wallet-signing-prod

dd12bda

refactor: .map().filter().map() => .flatMap()

a018c24

mergify bot merged commit e7e6529 into master Aug 6, 2022

mergify bot deleted the dc-wallet-signing-prod branch August 6, 2022 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(wallet/ui): interactive, background signing #5877

feat(wallet/ui): interactive, background signing #5877

dckc commented Aug 2, 2022 •

edited

Loading

dckc commented Aug 3, 2022 •

edited

Loading

samsiegart left a comment

samsiegart Aug 4, 2022

dckc Aug 5, 2022

dckc commented Aug 5, 2022

michaelfig left a comment

michaelfig Aug 5, 2022

michaelfig Aug 5, 2022

michaelfig Aug 5, 2022

michaelfig Aug 5, 2022

michaelfig Aug 5, 2022

dckc Aug 6, 2022 •

edited

Loading

turadg left a comment

turadg Aug 5, 2022

dckc Aug 5, 2022

turadg Aug 5, 2022 •

edited

Loading

turadg Aug 5, 2022

dckc Aug 6, 2022

turadg Aug 5, 2022

samsiegart Aug 5, 2022

dckc Aug 6, 2022

turadg Aug 5, 2022

turadg Aug 5, 2022

turadg Aug 5, 2022

dckc Aug 6, 2022

turadg Aug 5, 2022

dckc Aug 6, 2022

turadg Aug 5, 2022

turadg Aug 5, 2022

turadg Aug 6, 2022

dckc Aug 6, 2022

		@@ -0,0 +1,26 @@
		// @ts-check

		/** @type {import('@keplr-wallet/types').Currency} */

	export const STORAGE_KEY = 'agoric.eis0Aigi';
	export const BROWSER_LOCAL_STORAGE_KEY = 'agoric.eis0Aigi';

-export async function suggestChain(
-  networkConfig,
-  /** @type {string} */ caption = undefined,
-  io = {},
-) {
+/** @type {{
+  * networkConfig: NetworkConfig
+  * caption?: string
+  * io?: Partial<IoPowers>
+  * }}
+  */
+export async function suggestChain(networkConfig,, caption, io = {}) {

feat(wallet/ui): interactive, background signing #5877

feat(wallet/ui): interactive, background signing #5877

Conversation

dckc commented Aug 2, 2022 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

dckc commented Aug 3, 2022 • edited Loading

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

dckc commented Aug 5, 2022

michaelfig 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

dckc Aug 6, 2022 • edited Loading

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Aug 5, 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

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

dckc commented Aug 2, 2022 •

edited

Loading

dckc commented Aug 3, 2022 •

edited

Loading

dckc Aug 6, 2022 •

edited

Loading

turadg Aug 5, 2022 •

edited

Loading