new smart wallet #6084

turadg · 2022-08-29T19:06:34Z

closes: #4488

Description

A whole new Smart Wallet contract. This builds on the design and code of the agsolo wallet but the fresh start reduces the code surface to audit for PSM launch. It's also designed from the ground up for POLA and to take advantage of marshalling contexts instead of board ids.

Some features that aren't needed for PSM launch have been omitted and filed for future work:

And these are still needed for PSM launch:

Security Considerations

This let's anyone send a message to the contract that can execute arbitrary offers on a wallet instance. It relies on the bridge handler to ensure that the only messages that reach a wallet come from its owner.

Documentation Considerations

New docs including an attackers guide.

Testing Considerations

New tests

dckc · 2022-08-31T22:10:25Z

Let's please be sure numWantsSatisfied is in offer status records:
#4809 (comment)

packages/wallet/contract/package.json

dckc

solid progress!

packages/wallet/contract/src/smartWallet.js

packages/wallet/contract/src/walletFactory.js

dckc · 2022-09-01T02:36:24Z

packages/wallet/contract/test/test-psm.js

+  );
+
+  // @ts-expect-error faulty generic typedef
+  wallet.executeOffer(offerSpecCapData);


await? ah. no. We shouldn't rely on getting a promise for the result. Seems worth documenting with void if not a comment.

you mean document at the callsite? the docs on executeOffer already say it's a synchronous function:

* @returns {void} when the offer has been sent to Zoe; payouts go into this wallet's purses

dckc · 2022-09-01T04:58:37Z

@turadg here's hoping the results of this work don't have memory performance issues such as

smart wallet hangs on to kernel objects - seats, payments, invitations, ... #6104

samsiegart · 2022-09-02T18:13:21Z

packages/inter-protocol/src/psm/psm.js

@@ -149,7 +149,7 @@ export const start = async (zcf, privateArgs, baggage) => {
  /**
   * @param {ZCFSeat} seat
   * @param {Amount<'nat'>} given
-   * @param {Amount<'nat'>} [wanted]
+   * @param {Amount<'nat'>} [wanted] defaults to maximum anchor (given exchange rate minus fees)


Just curious, when would you want anything less than the maximum?

not sure myself. A case I can imagine is that if you want a certain amount and you don't know the fees you give a surplus and rely on the contract to only take what's necessary for the want.

packages/wallet/contract/src/invitations.js

packages/wallet/contract/src/smartWallet.js

packages/wallet/contract/src/utils.js

samsiegart · 2022-09-02T18:23:59Z

packages/wallet/contract/src/utils.js

+/**
+ *
+ * @param {AmountKeywordRecord} amountKeywordRecord
+ * @param {(brand: Brand) => ERef<Purse>} purseForBrand


Nit: Rename to getPurseForBrand (here and below). I thought that this was just a single purse reference based on the name.

The function does just look up a single purse for a single brand.

packages/wallet/contract/src/utils.js

packages/wallet/contract/src/smartWallet.js

samsiegart · 2022-09-02T18:29:34Z

packages/wallet/contract/src/smartWallet.js

+     * @returns {void} when the offer has been sent to Zoe; payouts go into this wallet's purses
+     * @throws if any parts of the offer can be determined synchronously to be invalid
+     */
+    executeOffer: capData => {


I guess this purposely doesn't handle attestations for now right?

yeah, not in scope for PSM launch. @dckc will supporting that affect the API?

@samsiegart remind me how we represented "this offer should include an attestation" in our acceptOffer argument?

dckc

just a few comments here and there. I haven't managed to read through the whole thing.

packages/wallet/contract/src/smartWallet.js

dckc · 2022-09-03T01:33:09Z

packages/wallet/contract/src/smartWallet.js

+   *
+   * @type {MapStore<Brand, ERef<Purse<'set'>>>}
+   */
+  const brandPurses = makeScalarBigMapStore('brand purses', { durable: true });


It still seems like we could reduce the scope of this by, for example, moving it along with publishPurses later in the file.

Good idea. For auditability I tried to gather the state and for upgradability I tried to gather precious state. But it would be better to have a comment explicitly about state requirements and let them be defined in the most advantageous place for safety.

dckc

oops; forgot to submit these comments...

packages/smart-wallet/src/walletFactory.js

packages/smart-wallet/src/smartWallet.js

dckc

Great work!

packages/smart-wallet/package.json

packages/smart-wallet/src/utils.js

turadg force-pushed the ta/split-smartwallet branch 2 times, most recently from 930e7de to eb052cd Compare August 31, 2022 21:49

turadg requested review from dckc and michaelfig August 31, 2022 21:49

dckc reviewed Sep 1, 2022

View reviewed changes

packages/wallet/contract/package.json Outdated Show resolved Hide resolved

dckc reviewed Sep 1, 2022

View reviewed changes

turadg force-pushed the ta/split-smartwallet branch 2 times, most recently from 1a58fc0 to cfd2b19 Compare September 2, 2022 17:42

samsiegart reviewed Sep 2, 2022

View reviewed changes

turadg force-pushed the ta/split-smartwallet branch from 51a456a to c74df81 Compare September 2, 2022 22:48

dckc reviewed Sep 3, 2022

View reviewed changes

This was referenced Sep 3, 2022

deprecate existing smart-wallet package #6121

Merged

refactorings for new smart-wallet #6122

Merged

turadg force-pushed the ta/split-smartwallet branch 2 times, most recently from 75371b5 to b63bf9f Compare September 3, 2022 23:33

turadg mentioned this pull request Sep 3, 2022

Support multiple purses per brand #6126

Open

turadg requested review from dckc and samsiegart September 3, 2022 23:44

turadg changed the title ~~ta/split smartwallet~~ new smart wallet Sep 3, 2022

turadg mentioned this pull request Sep 4, 2022

support new smart wallet in Wallet UI #6127

Closed

turadg marked this pull request as ready for review September 4, 2022 00:42

This was referenced Sep 4, 2022

refactorings to support new smart wallet #6128

Merged

6044 wallet voting #6077

Closed

[story] Support AMM trading in new smart wallet #6132

Open

turadg force-pushed the ta/split-smartwallet branch 2 times, most recently from 6c2c725 to d70db59 Compare September 4, 2022 21:28

turadg mentioned this pull request Sep 4, 2022

Wallet UI working with new Smart Wallet contract #6134

Merged

dckc mentioned this pull request Sep 6, 2022

WIP: smart wallet / PSM governance integration #6139

Closed

dckc reviewed Sep 6, 2022

View reviewed changes

turadg force-pushed the ta/split-smartwallet branch 2 times, most recently from 20f897d to abfb448 Compare September 6, 2022 19:24

dckc requested a review from dtribble September 7, 2022 17:19

turadg force-pushed the ta/split-smartwallet branch from c4c5009 to 5b570fb Compare September 7, 2022 17:37

ci(eslint): disable no-undef in .ts files

d3ba423

turadg force-pushed the ta/split-smartwallet branch from 5b570fb to 994eed3 Compare September 7, 2022 18:56

turadg requested a review from dckc September 7, 2022 18:58

turadg added the automerge:no-update (expert!) Automatically merge without updates label Sep 7, 2022

dckc approved these changes Sep 7, 2022

View reviewed changes

packages/smart-wallet/package.json Outdated Show resolved Hide resolved

packages/smart-wallet/src/utils.js Show resolved Hide resolved

turadg added 3 commits September 7, 2022 12:15

docs(zoe): fix link

bd77eb9

refactor(vats): wallet config into 'internal'

7920543

chore(ertp): parameterize makeEmptyPurse type

eb81837

turadg force-pushed the ta/split-smartwallet branch from 994eed3 to 9b85ce3 Compare September 7, 2022 19:16

feat: new Smart Wallet

708972f

turadg force-pushed the ta/split-smartwallet branch from 9b85ce3 to 708972f Compare September 7, 2022 20:00

mergify bot merged commit 1feaac3 into master Sep 7, 2022

mergify bot deleted the ta/split-smartwallet branch September 7, 2022 21:12

Chris-Hibbert mentioned this pull request Sep 7, 2022

fix(smart-wallet): invitation issuer is remote #6155

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new smart wallet #6084

new smart wallet #6084

turadg commented Aug 29, 2022 •

edited

Loading

dckc commented Aug 31, 2022

dckc left a comment

dckc Sep 1, 2022

turadg Sep 1, 2022

dckc commented Sep 1, 2022

samsiegart Sep 2, 2022

turadg Sep 2, 2022

samsiegart Sep 2, 2022

turadg Sep 3, 2022

samsiegart Sep 2, 2022

turadg Sep 2, 2022

dckc Sep 3, 2022

dckc left a comment

dckc Sep 3, 2022

turadg Sep 3, 2022

dckc left a comment

dckc left a comment

new smart wallet #6084

new smart wallet #6084

Conversation

turadg commented Aug 29, 2022 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

dckc commented Aug 31, 2022

dckc 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 Sep 1, 2022

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

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

turadg commented Aug 29, 2022 •

edited

Loading