Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support offer signing with keplr #28

Merged
merged 8 commits into from
Aug 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/rpc/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@
},
"dependencies": {
"vite": "^4.3.2",
"vite-tsconfig-paths": "^4.2.0"
"vite-tsconfig-paths": "^4.2.0",
"@endo/marshal": "^0.8.8"
},
"devDependencies": {
"@typescript-eslint/eslint-plugin": "^5.35.1",
Expand All @@ -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"
}
}
15 changes: 10 additions & 5 deletions packages/rpc/src/chainStorageWatcher.ts
Original file line number Diff line number Diff line change
@@ -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';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import { AgoricChainStoragePathKind } from './types';
import { batchVstorageQuery, keyToPath, pathToKey } from './batchQuery';
import type { UpdateHandler } from './types';

type Subscriber<T> = {
onUpdate: UpdateHandler<T>;
Expand Down Expand Up @@ -40,8 +40,8 @@ export type ChainStorageWatcher = ReturnType<
* requests for efficiency.
* @param rpcAddr RPC server URL
* @param chainId the chain id to use
* @param unmarshal CapData unserializer to use
* @param onError
* @param marshaller CapData marshal to use
* @param newPathQueryDelayMs
* @param refreshLowerBoundMs
* @param refreshUpperBoundMs
Expand All @@ -50,8 +50,8 @@ export type ChainStorageWatcher = ReturnType<
export const makeAgoricChainStorageWatcher = (
rpcAddr: string,
chainId: string,
unmarshal: FromCapData<string>,
onError?: (e: Error) => void,
marshaller = makeClientMarshaller(),
newPathQueryDelayMs = defaults.newPathQueryDelayMs,
refreshLowerBoundMs = defaults.refreshLowerBoundMs,
refreshUpperBoundMs = defaults.refreshUpperBoundMs,
Expand Down Expand Up @@ -105,7 +105,11 @@ export const makeAgoricChainStorageWatcher = (
}

try {
const data = await batchVstorageQuery(rpcAddr, unmarshal, paths);
const data = await batchVstorageQuery(
rpcAddr,
marshaller.fromCapData,
paths,
);
watchedPathsToSubscribers.forEach((subscribers, path) => {
// Path was watched after query fired, wait until next round.
if (!data[path]) return;
Expand Down Expand Up @@ -201,5 +205,6 @@ export const makeAgoricChainStorageWatcher = (
watchLatest,
chainId,
rpcAddr,
marshaller,
};
};
40 changes: 40 additions & 0 deletions packages/rpc/src/marshal.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { Far, makeMarshal } from '@endo/marshal';

const makeTranslationTable = (
makeSlot: (val: unknown, size: number) => unknown,
makeVal: (slot: unknown, iface: string | undefined) => unknown,
) => {
const valToSlot = new Map();
const slotToVal = new Map();

const convertValToSlot = (val: unknown) => {
if (valToSlot.has(val)) return valToSlot.get(val);
const slot = makeSlot(val, valToSlot.size);
valToSlot.set(val, slot);
slotToVal.set(slot, val);
return slot;
};

const convertSlotToVal = (slot: unknown, iface: string | undefined) => {
if (slot === null) return makeVal(slot, iface);
if (slotToVal.has(slot)) return slotToVal.get(slot);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean we should just do something like:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const val = makeVal(slot, iface);
valToSlot.set(val, slot);
slotToVal.set(slot, val);
return val;
};

return harden({ convertValToSlot, convertSlotToVal });
};

const synthesizeRemotable = (_slot: unknown, iface: string | undefined) =>
Far((iface ?? '').replace(/^Alleged: /, ''), {});

const { convertValToSlot, convertSlotToVal } = makeTranslationTable(slot => {
throw new Error(`unknown id: ${slot}`);
}, synthesizeRemotable);

export const makeClientMarshaller = () =>
makeMarshal(convertValToSlot, convertSlotToVal, {
serializeBodyFormat: 'smallcaps',
});
12 changes: 11 additions & 1 deletion packages/rpc/test/chainStorageWatcher.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,22 @@ const unmarshal = (val: unknown) => val;

let watcher: ReturnType<typeof makeAgoricChainStorageWatcher>;

vi.mock('../src/marshal', () => ({ makeMarshal: () => {} }));

describe('makeAgoricChainStorageWatcher', () => {
beforeEach(() => {
watcher = makeAgoricChainStorageWatcher(
fakeRpcAddr,
fakeChainId,
unmarshal,
undefined,
{
fromCapData: unmarshal,
// @ts-expect-error mock doesnt require capdata type
toCapData: marshal,
unserialize: unmarshal,
// @ts-expect-error mock doesnt require capdata type
serialize: marshal,
},
);
vi.useFakeTimers();
});
Expand Down
5 changes: 3 additions & 2 deletions packages/web-components/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"@agoric/smart-wallet": "^0.5.3",
"@agoric/wallet": "^0.18.3",
"@endo/captp": "^3.1.1",
"@endo/eventual-send": "^0.17.2",
"@endo/eventual-send": "^0.17.5",
"@endo/marshal": "^0.8.5",
"@endo/promise-kit": "^0.2.56",
"@lit-labs/react": "^1.0.1",
Expand All @@ -40,7 +40,8 @@
"eslint": "^8.36.0",
"eslint-plugin-lit": "^1.8.2",
"eslint-plugin-lit-a11y": "^2.4.0",
"vitest": "^0.32.0"
"vitest": "^0.34.1",
"ses": "0.18.7"
},
"customElements": "custom-elements.json",
"eslintConfig": {
Expand Down
38 changes: 38 additions & 0 deletions packages/web-components/src/wallet-connection/chainInfo.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/** @typedef {import('@keplr-wallet/types').Bech32Config} Bech32Config */
/** @typedef {import('@keplr-wallet/types').FeeCurrency} FeeCurrency */

/** @type {FeeCurrency} */
export const stakeCurrency = {
coinDenom: 'BLD',
coinMinimalDenom: 'ubld',
coinDecimals: 6,
coinGeckoId: undefined,
gasPriceStep: {
low: 0,
average: 0,
high: 0,
},
};

/** @type {FeeCurrency} */
export const stableCurrency = {
coinDenom: 'IST',
coinMinimalDenom: 'uist',
coinDecimals: 6,
coinGeckoId: undefined,
gasPriceStep: {
low: 0,
average: 0,
high: 0,
},
};

/** @type {Bech32Config} */
export const bech32Config = {
bech32PrefixAccAddr: 'agoric',
bech32PrefixAccPub: 'agoricpub',
bech32PrefixValAddr: 'agoricvaloper',
bech32PrefixValPub: 'agoricvaloperpub',
bech32PrefixConsAddr: 'agoricvalcons',
bech32PrefixConsPub: 'agoricvalconspub',
};
156 changes: 156 additions & 0 deletions packages/web-components/src/wallet-connection/makeInteractiveSigner.js
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
import { fromBech32, toBech32, fromBase64, toBase64 } from '@cosmjs/encoding';
import { Registry } from '@cosmjs/proto-signing';
import {
AminoTypes,
defaultRegistryTypes,
assertIsDeliverTxSuccess,
createBankAminoConverters,
createAuthzAminoConverters,
} from '@cosmjs/stargate';
import { MsgWalletSpendAction } from '@agoric/cosmic-proto/swingset/msgs.js';
import { stableCurrency, bech32Config } from './chainInfo.js';
import { Errors } from '../errors.js';

/** @typedef {import("@cosmjs/proto-signing").EncodeObject} EncodeObject */
/** @typedef {import("@cosmjs/stargate").AminoConverters} AminoConverters */
/** @typedef {import("@cosmjs/stargate").StdFee} StdFee */
/** @typedef {import('@keplr-wallet/types').ChainInfo} ChainInfo */
/** @typedef {import('@keplr-wallet/types').Keplr} Keplr */

/**
* @param {string} address
* @returns {Uint8Array}
*/
const toAccAddress = address => {
return fromBech32(address).data;
};

/**
* `/agoric.swingset.XXX` matches package agoric.swingset in swingset/msgs.proto
* aminoType taken from Type() in golang/cosmos/x/swingset/types/msgs.go
*/
const SwingsetMsgs = {
MsgWalletSpendAction: {
typeUrl: '/agoric.swingset.MsgWalletSpendAction',
aminoType: 'swingset/WalletSpendAction',
},
};

/** @typedef {{owner: string, spendAction: string}} WalletSpendAction */

const SwingsetRegistry = new Registry([
...defaultRegistryTypes,
// XXX should this list be "upstreamed" to @agoric/cosmic-proto?
[SwingsetMsgs.MsgWalletSpendAction.typeUrl, MsgWalletSpendAction],
]);

/**
* @returns {StdFee}
*/
const zeroFee = () => {
const { coinMinimalDenom: denom } = stableCurrency;
const fee = {
amount: [{ amount: '0', denom }],
gas: '300000', // TODO: estimate gas?
};
return fee;
};

/**
* @type {AminoConverters}
*/
const SwingsetConverters = {
[SwingsetMsgs.MsgWalletSpendAction.typeUrl]: {
aminoType: SwingsetMsgs.MsgWalletSpendAction.aminoType,
toAmino: ({ spendAction, owner }) => ({
spend_action: spendAction,
owner: toBech32(bech32Config.bech32PrefixAccAddr, fromBase64(owner)),
}),
fromAmino: ({ spend_action: spendAction, owner }) => ({
spendAction,
owner: toBase64(toAccAddress(owner)),
}),
},
};

/**
* Use Keplr to sign offers and delegate object messaging to local storage key.
* @param {string} chainId
* @param {string} rpc
* @param {Keplr} keplr
* @param {typeof import('@cosmjs/stargate').SigningStargateClient.connectWithSigner} connectWithSigner
Comment on lines +80 to +81
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* Ref: https://docs.keplr.app/api/
*/
export const makeInteractiveSigner = async (
chainId,
rpc,
keplr,
connectWithSigner,
) => {
await null;
try {
await keplr.enable(chainId);
} catch {
throw Error(Errors.enableKeplr);
}

const key = await keplr.getKey(chainId);

// Until we have SIGN_MODE_TEXTUAL,
// Use Amino because Direct results in ugly protobuf in the keplr UI.
const offlineSigner = await keplr.getOfflineSignerOnlyAmino(chainId);
console.debug('InteractiveSigner', { offlineSigner });

// Currently, Keplr extension manages only one address/public key pair.
const [account] = await offlineSigner.getAccounts();
const { address } = account;

const converters = {
...SwingsetConverters,
...createBankAminoConverters(),
...createAuthzAminoConverters(),
};
const signingClient = await connectWithSigner(rpc, offlineSigner, {
aminoTypes: new AminoTypes(converters),
registry: SwingsetRegistry,
});
console.debug('InteractiveSigner', { signingClient });

const fee = zeroFee();

return harden({
address, // TODO: address can change
isNanoLedger: key.isNanoLedger,
/**
* Sign and broadcast WalletSpendAction
*
* @param {string} spendAction marshaled offer
* @throws if account does not exist on chain, user cancels,
* RPC connection fails, RPC service fails to broadcast (
* for example, if signature verification fails)
*/
submitSpendAction: async spendAction => {
const { accountNumber, sequence } = await signingClient.getSequence(
address,
);
console.debug({ accountNumber, sequence });

const act1 = {
typeUrl: SwingsetMsgs.MsgWalletSpendAction.typeUrl,
value: {
owner: toBase64(toAccAddress(address)),
spendAction,
},
};

const msgs = [act1];
console.debug('sign spend action', { address, msgs, fee });

const tx = await signingClient.signAndBroadcast(address, msgs, fee);
console.debug('spend action result tx', tx);
assertIsDeliverTxSuccess(tx);

return tx;
},
});
};