feat(smart-wallet): exit offer #7028

samsiegart · 2023-02-17T23:20:39Z

closes: #6906

Description

Makes it possible for clients to tryExit a Zoe seat. For them to know what seats are live it also publishes liveSeats on the smart wallet's .current record.

Security Considerations

Publishing status off live offers, but nothing precious.

Scaling Considerations

Iterates over all liveSeats to publish them. Strictly speaking this goes against the limit work by message size rule. But as discussed in #6184, there are somewhat conflicting requirements here and this is the best solution on balance.

Documentation Considerations

--

Testing Considerations

New test exit bid

samsiegart · 2023-02-18T05:38:15Z

packages/inter-protocol/test/smartWallet/test-psm-integration.js

+    updated: 'balance',
+  });
+
+  const statusUpdateHasKeys = (updateIndex, result, numWants, payouts) => {


@turadg I reverted these asserts because they only worked when the test was run in isolation, and we ended up changing current not updates

samsiegart · 2023-02-18T05:42:28Z

packages/smart-wallet/src/smartWallet.js

@@ -124,6 +124,7 @@ const mapToRecord = map => Object.fromEntries(map.entries());
 *   purseBalances: MapStore<RemotePurse, Amount>,
 *   updatePublishKit: PublishKit<UpdateRecord>,
 *   currentPublishKit: PublishKit<CurrentWalletRecord>,
+ *   possiblyExitableOffers: MapStore<import('./offers.js').OfferId, import('./offers.js').OfferStatus>,


@turadg I renamed this because:

It makes it easier to understand why we're storing them.

I don't think it's the exact same as currently seated offers. If we have an error, the offer could still be seated, but at that point the smart wallet should stop storing it and try to unseat it automatically.

After my latest changes, point 2 is irrelevant though. We need to still store it in case the exit fails due to a deadline or something.

I forget what the earlier term was but this one feels klunky. let's chat higher bandwidth

samsiegart · 2023-02-18T07:12:52Z

packages/smart-wallet/src/offers.js

@@ -111,7 +116,6 @@ export const makeOfferExecutor = ({
          offerArgs,
        );
        logger.info(id, 'seated');
-        updateStatus({});


I included "exit offer" in this PR because it influenced the design here. We return the seat if this succeeds rather than updating the status with {}.

I can take this commit out of this PR though if needed.

samsiegart · 2023-02-18T07:15:08Z

packages/smart-wallet/src/smartWallet.js

@@ -124,6 +135,8 @@ const mapToRecord = map => Object.fromEntries(map.entries());
 *   purseBalances: MapStore<RemotePurse, Amount>,
 *   updatePublishKit: PublishKit<UpdateRecord>,
 *   currentPublishKit: PublishKit<CurrentWalletRecord>,
+ *   possiblyExitableOffers: MapStore<import('./offers.js').OfferId, import('./offers.js').OfferStatus>,
+ *   activeOfferSeats: MapStore<import('./offers.js').OfferId, UserSeat<unknown>>,


I suppose we could store the seats in possiblyExitableOffers and filter them out when we publish current. I'm not sure how much of a performance difference it is.

Good question. Compute vs storage, I don't know how those costs compare on the chain. My hunch is that we shouldn't be using durable storage as a cache of computed results but let's get more input.

samsiegart · 2023-02-18T09:02:45Z

packages/smart-wallet/src/smartWallet.js

@@ -476,9 +476,8 @@ export const prepareSmartWallet = (baggage, shared) => {
              });

              const isOfferPossiblyExitable = !(
-                'error' in offerStatus || 'numWantsSatisfied' in offerStatus


If there is an error, we don't necessarily want to forget about it just yet. The offer executor should automatically try to exit the offer in that case, and then numWantsSatisfied should come back as 0. However, the exit could fail if the rule is "after deadline" or something, so in that case keep it around so the user can manually try to exit it later.

samsiegart · 2023-02-22T00:41:26Z

Tested with Agoric/wallet-app#62 and vaults on a local chain and was able to view/exit currently seated offers in the wallet UI

turadg · 2023-02-24T23:30:45Z

packages/smart-wallet/src/offers.js

       */
-      const handleError = err => {
+      const handleError = (err, seatRef) => {


this optional seatRef is a little loose. Instead leave this handleError alone and make a new function like exitSeatAndHandleError in the tryBody scope. It won't need to take seatRef because it's in the closure. Then the error handler lines can continue to be a prebound function instead of a new one for each when.

Also better to exit the offer before reclaiming payments. I don't know if it affects anything in practice but it's conceptually more clear.

turadg · 2023-02-24T23:36:13Z

packages/smart-wallet/src/offers.js

+          E(seatRef)
+            .tryExit()
+            .catch(e => {
+              logger.error('EXIT OFFER ERROR:', e);


this isn't an error if the seat was already exited. generally the contracts should do that. This is a backstop.

let's only tryExit if it hasn't already exited (chain with the hasExited() promise). Then an exception would be an error. A pretty big one though so I think we'd want to throw instead of merely catch and log.

turadg · 2023-02-24T23:37:15Z

packages/smart-wallet/src/smartWallet.js

+ * }} TryExitOfferAction
+ */
+
+// One method yet but structured to support more. For example,


there are now two methods. the part about additional potential ones is worth keeping.

turadg · 2023-02-24T23:37:28Z

packages/smart-wallet/src/smartWallet.js

+ * @typedef {{
+ *   method: 'tryExitOffer'
+ *   offerId: import('./offers.js').OfferId,
+ * }} TryExitOfferAction


turadg · 2023-02-24T23:39:26Z

packages/smart-wallet/src/smartWallet.js

@@ -124,6 +124,7 @@ const mapToRecord = map => Object.fromEntries(map.entries());
 *   purseBalances: MapStore<RemotePurse, Amount>,
 *   updatePublishKit: PublishKit<UpdateRecord>,
 *   currentPublishKit: PublishKit<CurrentWalletRecord>,
+ *   possiblyExitableOffers: MapStore<import('./offers.js').OfferId, import('./offers.js').OfferStatus>,


I forget what the earlier term was but this one feels klunky. let's chat higher bandwidth

turadg · 2023-02-24T23:43:02Z

packages/smart-wallet/src/smartWallet.js

              updatePublishKit.publisher.publish({
                updated: 'offerStatus',
                status: offerStatus,
              });
+
+              const isOfferPossiblyExitable = !(


there's got to be a better name for this state. "unresolved"? @Chris-Hibbert or @dckc may have ideas

I looked in our UserSeat docs, and I'm still not clear on all the possible states and transitions, let alone good names for them.

"pending" is the first word that came to mind, but I don't know whether it's accurate.

I'd offer 'live' or 'open'. The alternative is exited.

turadg · 2023-02-24T23:43:47Z

packages/smart-wallet/src/smartWallet.js

+              state.activeOfferSeats.has(offerStatus.id) &&
+                state.activeOfferSeats.delete(offerStatus.id);


this isn't frontend :-P

Suggested change

state.activeOfferSeats.has(offerStatus.id) &&

state.activeOfferSeats.delete(offerStatus.id);

if (state.activeOfferSeats.has(offerStatus.id)) {

state.activeOfferSeats.delete(offerStatus.id);

}

Should we use https://eslint.org/docs/latest/rules/no-unused-expressions then?

Good idea. Its defaults ban more than we'd want but the settings look like it could be narrowed appropriately.

Do you want to PR that or should I?

Also, thanks for looking for a way to automate the review comment. Always better to let the tools do it.

turadg · 2023-02-24T23:49:12Z

packages/smart-wallet/test/test-walletFactory.js

@@ -160,4 +160,5 @@ test.todo(
  // pause the PSM trading such that there is time to exit before offer resolves
  // executeOffer to buy the junk (which can't resolve)
  // exit the offer "oh I don't want to buy junk!"
+  // Help?


let's pair on Monday

prune this? (I got distracted)

turadg · 2023-02-24T23:49:58Z

packages/smart-wallet/src/smartWallet.js

+         * @throws if the seat can't be found or E(seatRef).tryExit() fails.
+         */
+        async tryExitOffer(offerId) {
+          const seatRef = this.state.activeOfferSeats.get(offerId);


I'm confused why we have possiblyExitableOffers and don't use it here

turadg · 2023-02-24T23:53:01Z

packages/smart-wallet/src/smartWallet.js

@@ -468,7 +507,23 @@ export const prepareSmartWallet = (baggage, shared) => {
              facets.helper.publishCurrentState();
            },
          });
-          await executor.executeOffer(offerSpec);
+          const seatRef = await executor.executeOffer(offerSpec);


the return is null only if an error happens. WDYT of a try/catch instead?

dckc · 2023-02-25T00:08:27Z

packages/smart-wallet/src/smartWallet.js

          } = this.state;
          currentPublishKit.publisher.publish({
            purses: [...purseBalances.values()].map(a => ({
              brand: a.brand,
              balance: a,
            })),
+            possiblyExitableOffers: mapToRecord(possiblyExitableOffers),


Let's not use mapToRecord any more; we're trying to get rid of it.

oh... what to use instead...

Suggested change

possiblyExitableOffers: mapToRecord(possiblyExitableOffers),

possiblyExitableOffers: possiblyExitableOffers.entries(),

Oops... I suppose .entries() returns an interator. We should materialize the list:

Suggested change

possiblyExitableOffers: mapToRecord(possiblyExitableOffers),

possiblyExitableOffers: [...possiblyExitableOffers.entries()],

Chris-Hibbert · 2023-02-25T01:02:26Z

Does the smartWallet already include an exit clause by default (or choice) when making an offer? Some contracts will refuse exit clauses they don't like, so I think this would have to be chosen by the user or configured by the contract. Without an appropriate exit clause (exit: { onDemand: null }), the smartWallet (or anything else) won't be able to tryExit().

datadog-full-agoric · 2023-03-21T18:09:39Z

Datadog Report

Branch report: publish-pending-offers
Commit report: 6abefaf

✅ agoric-sdk: 0 Failed, 0 New Flaky, 3812 Passed, 57 Skipped, 27m 12.71s Wall Time

Chris-Hibbert

some comments/questions as I drove by. I didn't see anything objectionable.
Please don't count me as a primary reviewer.

Chris-Hibbert · 2023-03-22T16:11:59Z

packages/inter-protocol/src/auction/auctionBook.js

@@ -48,7 +48,7 @@ const DEFAULT_DECIMALS = 9;
 * added in the appropriate place and settled when the price reaches that level.
 */

-const trace = makeTracer('AucBook', false);
+const trace = makeTracer('AucBook');


Do you intend for this change to be merged?

That does look odd.

it's been useful for debugging. I expect we'll audit all tracers before cutting the release

I expect we'll audit all tracers before cutting the release

Well, here's hoping.

#5222 doesn't seem to be scheduled, though.

Thanks for the issue. I put it in the release. I expect it'll get triaged.

Chris-Hibbert · 2023-03-22T16:25:13Z

packages/smart-wallet/src/offers.js

+          E(seatRef)
+            .hasExited()
+            .then(hasExited => {
+              if (!hasExited) {
+                E(seatRef).tryExit();
+              }
+            });


MarkM seems to prefer E.when() to .then().

Suggested change

E(seatRef)

.hasExited()

.then(hasExited => {

if (!hasExited) {

E(seatRef).tryExit();

}

});

E.when(

E(seatRef).hasExited(),

hasExited => {

if (!hasExited) {

E(seatRef).tryExit();

}

},

);

Using E.when() always is easier than thinking, but in this case I'm not going to insist: The risk is that the supposed-promise from hasExited() would be have a bogus then method, but it comes from zoe, and the contract pretty much has to assume zoe is correct; that is: it relies on zoe. In fact, I think this promise comes from E, which the contract relies on even more.

Chris-Hibbert · 2023-03-22T16:30:08Z

packages/smart-wallet/src/smartWallet.js

+         */
+        async tryExitOffer(offerId) {
+          const seatRef = this.state.liveOfferSeats.get(offerId);
+          await E(seatRef).tryExit();


does it need to return await E(seatRef).tryExit(); in order to actually throw/break the returned promise on failure?

no. There are no good uses for return await x; it's equivalent to return x.

Under the normal async transformation, a function that ends with an await x expression statement is like one that ends with return x.then(_ => undefined).

Yes, my point wasn't that it should return await it was that it should return the results of .tryExit().

Does it need to return E(seatRef).tryExit(); in order to actually throw/break the returned promise on failure?

Does it need to return E(seatRef).tryExit(); in order to actually throw/break the returned promise on failure?

no, the await will propagate the promise rejection from E if the seat's tryExit throws.

codecov · 2023-03-22T19:28:16Z

Codecov Report

Merging #7028 (21cc31e) into master (f447e3a) will decrease coverage by 8.75%.
The diff coverage is 68.75%.

❗ Current head 21cc31e differs from pull request most recent head 97c7a71. Consider uploading reports for the commit 97c7a71 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7028      +/-   ##
==========================================
- Coverage   79.38%   70.64%   -8.75%     
==========================================
  Files         396      443      +47     
  Lines       74792    84631    +9839     
  Branches        3        3              
==========================================
+ Hits        59373    59785     +412     
- Misses      15418    24780    +9362     
- Partials        1       66      +65

Impacted Files	Coverage Δ
packages/internal/src/storage-test-utils.js	`62.50% <50.00%> (-3.54%)`	⬇️
packages/vats/src/core/lib-boot.js	`80.52% <50.00%> (-1.00%)`	⬇️
packages/inter-protocol/src/auction/auctionBook.js	`73.25% <100.00%> (-0.04%)`	⬇️
packages/inter-protocol/src/clientSupport.js	`47.47% <100.00%> (-0.21%)`	⬇️
packages/vats/tools/board-utils.js	`87.40% <100.00%> (ø)`

... and 80 files with indirect coverage changes

dckc

Good stuff.

I make a few somewhat strong suggestions in here... but not of them quite critical.

dckc · 2023-03-24T18:45:24Z

packages/smart-wallet/test/test-walletFactory.js

@@ -160,4 +160,5 @@ test.todo(
  // pause the PSM trading such that there is time to exit before offer resolves
  // executeOffer to buy the junk (which can't resolve)
  // exit the offer "oh I don't want to buy junk!"
+  // Help?


prune this? (I got distracted)

dckc · 2023-03-24T22:05:40Z

packages/inter-protocol/src/auction/auctionBook.js

@@ -48,7 +48,7 @@ const DEFAULT_DECIMALS = 9;
 * added in the appropriate place and settled when the price reaches that level.
 */

-const trace = makeTracer('AucBook', false);
+const trace = makeTracer('AucBook');


That does look odd.

dckc · 2023-03-24T22:06:32Z

packages/inter-protocol/src/auction/auctionBook.js

@@ -290,10 +291,12 @@ export const prepareAuctionBook = (baggage, zcf) => {
              : AmountMath.makeEmptyFromAmount(want);

          const stillWant = AmountMath.subtract(want, collateralSold);
+          trace('acceptScaledBidOffer', { stillWant });


Any particular reason we're adding tracing to auctionBook in order to add tryExit to the smart wallet?

used for debugging of the bid life cycle. the test of tryExit is exit bid.

dckc · 2023-03-24T22:08:37Z

packages/inter-protocol/src/auction/auctionBook.js

@@ -48,7 +48,7 @@ const DEFAULT_DECIMALS = 9;
 * added in the appropriate place and settled when the price reaches that level.
 */

-const trace = makeTracer('AucBook', false);
+const trace = makeTracer('AucBook');


I expect we'll audit all tracers before cutting the release

Well, here's hoping.

#5222 doesn't seem to be scheduled, though.

dckc · 2023-03-24T22:09:48Z

packages/inter-protocol/src/clientSupport.js

@@ -230,7 +229,7 @@ const makeBidOffer = (brands, opts) => {
      instancePath: ['auctioneer'],
      callPipe: [['makeBidInvitation', [collateralBrand]]],
    },
-    proposal,
+    proposal: { give, exit: { onDemand: null } },


dckc · 2023-03-24T22:33:11Z

packages/smart-wallet/src/smartWallet.js

+         */
+        async tryExitOffer(offerId) {
+          const seatRef = this.state.liveOfferSeats.get(offerId);
+          await E(seatRef).tryExit();


no. There are no good uses for return await x; it's equivalent to return x.

Under the normal async transformation, a function that ends with an await x expression statement is like one that ends with return x.then(_ => undefined).

packages/smart-wallet/src/utils.js

dckc · 2023-03-24T22:36:05Z

packages/vats/src/core/lib-boot.js

+    messageVatObjectSendOnly: ({ presence, methodName, args = [] }) => {
+      const object = decodePassable(presence);
+      const decodedArgs = args.map(decodePassable);
+      E(object)[methodName](...decodedArgs);


Suggested change

E(object)[methodName](...decodedArgs);

void E(object)[methodName](...decodedArgs);

dckc · 2023-03-24T22:36:45Z

packages/vats/test/bootstrapTests/supports.js

@@ -73,6 +74,7 @@ export const makeRunUtils = (controller, log = (..._) => {}) => {

  /**
   * @type {( (presence: unknown) => Record<string, (...args: any) => Promise<any>> ) & {
+   *   sendOnly: (presence: unknown) => Record<string, (...args: any) => any>,


Suggested change

* sendOnly: (presence: unknown) => Record<string, (...args: any) => any>,

* sendOnly: (presence: unknown) => Record<string, (...args: any) => void>,

dckc · 2023-03-24T22:40:51Z

packages/vats/test/bootstrapTests/supports.js

    /**
     * @returns {import('@agoric/smart-wallet/src/smartWallet.js').UpdateRecord}
     */
    getLatestUpdateRecord() {
      const key = `published.wallet.${walletAddress}`;
-      const lastWalletStatus = JSON.parse(storage.data.get(key).at(-1));
+      const lastWalletStatus = JSON.parse(storage.data.get(key)?.at(-1));


How does this ? help? JSON.parse(undefined) throws. Oh... but it seems to typecheck. weird. maybe a js-mode thing.

Co-authored-by: Turadg Aleahmad <[email protected]>

erights · 2023-03-28T06:41:09Z

Does the smartWallet already include an exit clause by default (or choice) when making an offer? Some contracts will refuse exit clauses they don't like, so I think this would have to be chosen by the user or configured by the contract. Without an appropriate exit clause (exit: { onDemand: null }), the smartWallet (or anything else) won't be able to tryExit().

If no exit clause is provided, cleanProposal will fill in the default exit: { onDemand: null }

samsiegart force-pushed the publish-pending-offers branch from 2635aaa to 17312dd Compare February 18, 2023 01:02

samsiegart changed the title ~~feat(smart-wallet): publish pending offers before completion~~ feat(smart-wallet): publish possibly exitable offers in current Feb 18, 2023

samsiegart force-pushed the publish-pending-offers branch from 17312dd to 94cb18b Compare February 18, 2023 05:32

samsiegart marked this pull request as ready for review February 18, 2023 05:35

samsiegart force-pushed the publish-pending-offers branch from 94cb18b to 94443a5 Compare February 18, 2023 05:45

samsiegart commented Feb 18, 2023

View reviewed changes

samsiegart requested a review from turadg February 18, 2023 05:46

samsiegart commented Feb 18, 2023

View reviewed changes

samsiegart force-pushed the publish-pending-offers branch from 784d15a to 784ea0a Compare February 18, 2023 09:00

samsiegart commented Feb 18, 2023

View reviewed changes

samsiegart changed the title ~~feat(smart-wallet): publish possibly exitable offers in current~~ feat(smart-wallet): exit offer Feb 18, 2023

samsiegart mentioned this pull request Feb 22, 2023

feat: allow exiting offers Agoric/wallet-app#62

Merged

turadg reviewed Feb 24, 2023

View reviewed changes

dckc reviewed Feb 25, 2023

View reviewed changes

samsiegart force-pushed the publish-pending-offers branch 2 times, most recently from 3a2c331 to c53710c Compare March 21, 2023 18:00

turadg requested review from dckc and Chris-Hibbert March 21, 2023 23:39

turadg self-assigned this Mar 22, 2023

turadg mentioned this pull request Mar 22, 2023

minimum bid for auction #7210

Merged

Chris-Hibbert reviewed Mar 22, 2023

View reviewed changes

dckc approved these changes Mar 24, 2023

View reviewed changes

turadg force-pushed the publish-pending-offers branch 2 times, most recently from 14d7295 to 2ba5c85 Compare March 24, 2023 23:48

turadg added the automerge:no-update (expert!) Automatically merge without updates label Mar 24, 2023

turadg and others added 5 commits March 27, 2023 09:13

test: support for EV.sendonly

1370654

feat(smart-wallet): publish pending offers before completion

c913b36

feat(smart-wallet): publish possibly exitable offers

de0170a

Co-authored-by: Turadg Aleahmad <[email protected]>

feat(smart-wallet): exit offer

7323023

test: exiting bid

97c7a71

turadg force-pushed the publish-pending-offers branch from 2ba5c85 to 97c7a71 Compare March 27, 2023 16:16

mergify bot merged commit 7989b88 into master Mar 27, 2023

mergify bot deleted the publish-pending-offers branch March 27, 2023 16:57

		state.activeOfferSeats.has(offerStatus.id) &&
		state.activeOfferSeats.delete(offerStatus.id);

	possiblyExitableOffers: mapToRecord(possiblyExitableOffers),
	possiblyExitableOffers: possiblyExitableOffers.entries(),

	possiblyExitableOffers: mapToRecord(possiblyExitableOffers),
	possiblyExitableOffers: [...possiblyExitableOffers.entries()],

	E(object)[methodName](...decodedArgs);
	void E(object)[methodName](...decodedArgs);

	* sendOnly: (presence: unknown) => Record<string, (...args: any) => any>,
	* sendOnly: (presence: unknown) => Record<string, (...args: any) => void>,

feat(smart-wallet): exit offer #7028

feat(smart-wallet): exit offer #7028

Conversation

samsiegart commented Feb 17, 2023 • edited by turadg Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

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

samsiegart commented Feb 22, 2023

turadg Feb 24, 2023 • edited by Chris-Hibbert 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

Chris-Hibbert commented Feb 25, 2023

datadog-full-agoric bot commented Mar 21, 2023 • edited Loading

Datadog Report

Chris-Hibbert 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

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

codecov bot commented Mar 22, 2023 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

turadg Mar 24, 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

erights commented Mar 28, 2023

samsiegart commented Feb 17, 2023 •

edited by turadg

Loading

turadg Feb 24, 2023 •

edited by Chris-Hibbert

Loading

datadog-full-agoric bot commented Mar 21, 2023 •

edited

Loading

codecov bot commented Mar 22, 2023 •

edited

Loading

turadg Mar 24, 2023 •

edited

Loading