improve error handling (#11070)

closes: #10968 ## Description - **feat: clean up upon last attest** - **feat: check conflicting evidence before quorum** - chore: clean up promise handling - replace debug() with trace() in PacketTools - **docs: TxFeed does not inspect evidence** - **test: self disagreement** ### Security Considerations slightly stricter check ### Scaling Considerations reduces growth of TxFeed's `pending` store (which would always have the third operator submission) ### Documentation Considerations some new docs ### Testing Considerations some new tests ### Upgrade Considerations Fully backwards compatible
Agoric · Mar 3, 2025 · 7efdf47 · 7efdf47
2 parents 9423fce + 741be79
commit 7efdf47
Show file tree

Hide file tree

Showing 9 changed files with 149 additions and 211 deletions.
diff --git a/packages/fast-usdc/src/exos/advancer.js b/packages/fast-usdc/src/exos/advancer.js
@@ -211,6 +211,8 @@ export const prepareAdvancerKit = (
               poolAccount,
               harden({ USDC: advanceAmount }),
             );
+            // WARNING: this must never reject, see handler @throws {never} below
+            // void not enforced by linter until #10627 no-floating-vows
             void watch(depositV, this.facets.depositHandler, {
               advanceAmount,
               destination,
@@ -233,6 +235,7 @@ export const prepareAdvancerKit = (
         /**
          * @param {undefined} result
          * @param {AdvancerVowCtx & { tmpSeat: ZCFSeat }} ctx
+         * @throws {never} WARNING: this function must not throw, because user funds are at risk
          */
         onFulfilled(result, ctx) {
           const { poolAccount, intermediateRecipient, settlementAddress } =
@@ -265,6 +268,7 @@ export const prepareAdvancerKit = (
          *
          * @param {Error} error
          * @param {AdvancerVowCtx & { tmpSeat: ZCFSeat }} ctx
+         * @throws {never} WARNING: this function must not throw, because user funds are at risk
          */
         onRejected(error, { tmpSeat, advanceAmount, ...restCtx }) {
           log(
@@ -285,17 +289,12 @@ export const prepareAdvancerKit = (
         /**
          * @param {undefined} result
          * @param {AdvancerVowCtx} ctx
+         * @throws {never} WARNING: this function must not throw, because user funds are at risk
          */
         onFulfilled(result, ctx) {
           const { notifier } = this.state;
           const { advanceAmount, destination, ...detail } = ctx;
           log('Advance succeeded', { advanceAmount, destination });
-          // During development, due to a bug, this call threw.
-          // The failure was silent (no diagnostics) due to:
-          //  - #10576 Vows do not report unhandled rejections
-          // For now, the advancer kit relies on consistency between
-          // notify, statusManager, and callers of handleTransactionEvent().
-          // TODO: revisit #10576 during #10510
           notifier.notifyAdvancingResult({ destination, ...detail }, true);
         },
         /**
@@ -314,6 +313,8 @@ export const prepareAdvancerKit = (
             tmpReturnSeat,
             harden({ USDC: advanceAmount }),
           );
+          // WARNING: this must never reject, see handler @throws {never} below
+          // void not enforced by linter until #10627 no-floating-vows
           void watch(withdrawV, this.facets.withdrawHandler, {
             advanceAmount,
             tmpReturnSeat,
@@ -325,6 +326,7 @@ export const prepareAdvancerKit = (
          *
          * @param {undefined} result
          * @param {{ advanceAmount: Amount<'nat'>; tmpReturnSeat: ZCFSeat; }} ctx
+         * @throws {never} WARNING: this function must not throw, because user funds are at risk
          */
         onFulfilled(result, { advanceAmount, tmpReturnSeat }) {
           const { borrower } = this.state;
@@ -342,6 +344,7 @@ export const prepareAdvancerKit = (
         /**
          * @param {Error} error
          * @param {{ advanceAmount: Amount<'nat'>; tmpReturnSeat: ZCFSeat; }} ctx
+         * @throws {never} WARNING: this function must not throw, because user funds are at risk
          */
         onRejected(error, { advanceAmount, tmpReturnSeat }) {
           log(

diff --git a/packages/fast-usdc/src/exos/status-manager.js b/packages/fast-usdc/src/exos/status-manager.js
@@ -117,9 +117,8 @@ export const prepareStatusManager = (
   /**
    * @param {EvmHash} txId
    * @param {TransactionRecord} record
-   * @returns {Promise<void>}
    */
-  const publishTxnRecord = async (txId, record) => {
+  const publishTxnRecord = (txId, record) => {
     const txNode = E(txnsNode).makeChildNode(txId, {
       sequence: true, // avoid overwriting other output in the block
     });
@@ -133,9 +132,10 @@ export const prepareStatusManager = (
       storedCompletedTxs.add(txId);
     }
 
-    const capData = await E(marshaller).toCapData(record);
-
-    await E(txNode).setValue(JSON.stringify(capData));
+    // Don't await, just writing to vstorage.
+    void E.when(E(marshaller).toCapData(record), capData =>
+      E(txNode).setValue(JSON.stringify(capData)),
+    );
   };
 
   /**
@@ -144,10 +144,7 @@ export const prepareStatusManager = (
    */
   const publishEvidence = (hash, evidence) => {
     // Don't await, just writing to vstorage.
-    void publishTxnRecord(
-      hash,
-      harden({ evidence, status: TxStatus.Observed }),
-    );
+    publishTxnRecord(hash, harden({ evidence, status: TxStatus.Observed }));
   };
 
   /**
@@ -174,10 +171,10 @@ export const prepareStatusManager = (
     );
     publishEvidence(txHash, evidence);
     if (status === PendingTxStatus.AdvanceSkipped) {
-      void publishTxnRecord(txHash, harden({ status, risksIdentified }));
+      publishTxnRecord(txHash, harden({ status, risksIdentified }));
     } else if (status !== PendingTxStatus.Observed) {
       // publishEvidence publishes Observed
-      void publishTxnRecord(txHash, harden({ status }));
+      publishTxnRecord(txHash, harden({ status }));
     }
   };
 
@@ -200,7 +197,7 @@ export const prepareStatusManager = (
     ];
     const txpost = { ...tx, status };
     pendingSettleTxs.set(key, harden([...prefix, txpost, ...suffix]));
-    void publishTxnRecord(tx.txHash, harden({ status }));
+    publishTxnRecord(tx.txHash, harden({ status }));
   }
 
   return zone.exo(
@@ -288,7 +285,7 @@ export const prepareStatusManager = (
        * @param {boolean} success whether the Transfer succeeded
        */
       advanceOutcomeForMintedEarly(txHash, success) {
-        void publishTxnRecord(
+        publishTxnRecord(
           txHash,
           harden({
             status: success
@@ -381,10 +378,7 @@ export const prepareStatusManager = (
        * @param {import('./liquidity-pool.js').RepayAmountKWR} split
        */
       disbursed(txHash, split) {
-        void publishTxnRecord(
-          txHash,
-          harden({ split, status: TxStatus.Disbursed }),
-        );
+        publishTxnRecord(txHash, harden({ split, status: TxStatus.Disbursed }));
       },
 
       /**
@@ -394,7 +388,7 @@ export const prepareStatusManager = (
        * @param {boolean} success
        */
       forwarded(txHash, success) {
-        void publishTxnRecord(
+        publishTxnRecord(
           txHash,
           harden({
             status: success ? TxStatus.Forwarded : TxStatus.ForwardFailed,

diff --git a/packages/fast-usdc/src/exos/transaction-feed.js b/packages/fast-usdc/src/exos/transaction-feed.js
@@ -1,7 +1,8 @@
+/** @file Exo for @see {prepareTransactionFeedKit} */
 import { makeTracer } from '@agoric/internal';
 import { prepareDurablePublishKit } from '@agoric/notifier';
-import { keyEQ, M } from '@endo/patterns';
 import { Fail, quote } from '@endo/errors';
+import { keyEQ, M } from '@endo/patterns';
 import { CctpTxEvidenceShape, RiskAssessmentShape } from '../type-guards.js';
 import { defineInertInvitation } from '../utils/zoe.js';
 import { prepareOperatorKit } from './operator-kit.js';
@@ -64,8 +65,14 @@ export const stateShape = {
   pending: M.remotable(),
   risks: M.remotable(),
 };
+harden(stateShape);
 
 /**
+ * A TransactionFeed is responsible for finding quorum among oracles.
+ *
+ * It receives attestations, records their evidence, and when enough oracles
+ * agree, publishes the results for the advancer to act on.
+ *
  * @param {Zone} zone
  * @param {ZCF} zcf
  */
@@ -148,18 +155,20 @@ export const prepareTransactionFeedKit = (zone, zcf) => {
 
         /** @param {string} operatorId */
         removeOperator(operatorId) {
-          const { operators } = this.state;
+          const { operators, pending, risks } = this.state;
           trace('removeOperator', operatorId);
           const operatorKit = operators.get(operatorId);
           operatorKit.admin.disable();
           operators.delete(operatorId);
+          pending.delete(operatorId);
+          risks.delete(operatorId);
         },
       },
       operatorPowers: {
         /**
          * Add evidence from an operator.
          *
-         * NB: the operatorKit is responsible for
+         * NB: the operatorKit is responsible for revoking access.
          *
          * @param {CctpTxEvidence} evidence
          * @param {RiskAssessment} riskAssessment
@@ -169,10 +178,6 @@ export const prepareTransactionFeedKit = (zone, zcf) => {
           const { operators, pending, risks } = this.state;
           trace('attest', operatorId, evidence);
 
-          // TODO https://github.com/Agoric/agoric-sdk/pull/10720
-          // TODO validate that it's a valid for Fast USDC before accepting
-          // E.g. that the `recipientAddress` is the FU settlement account and that
-          // the EUD is a chain supported by FU.
           const { txHash } = evidence;
 
           // accept the evidence
@@ -192,6 +197,29 @@ export const prepareTransactionFeedKit = (zone, zcf) => {
           const found = [...pending.values()].filter(store =>
             store.has(txHash),
           );
+
+          {
+            let lastEvidence;
+            for (const store of found) {
+              const next = store.get(txHash);
+              if (lastEvidence && !keyEQ(lastEvidence, next)) {
+                // Ignore conflicting evidence, but treat it as an error
+                // because it should never happen and needs to be prevented
+                // from happening again.
+                trace(
+                  '🚨 conflicting evidence for',
+                  txHash,
+                  ':',
+                  lastEvidence,
+                  '!=',
+                  next,
+                );
+                Fail`conflicting evidence for ${quote(txHash)}`;
+              }
+              lastEvidence = next;
+            }
+          }
+
           const minAttestations = Math.ceil(operators.getSize() / 2);
           trace(
             'transaction',
@@ -202,48 +230,37 @@ export const prepareTransactionFeedKit = (zone, zcf) => {
             minAttestations,
             'necessary attestations',
           );
+
           if (found.length < minAttestations) {
+            // wait for more
             return;
           }
 
-          let lastEvidence;
-          for (const store of found) {
-            const next = store.get(txHash);
-            if (lastEvidence && !keyEQ(lastEvidence, next)) {
-              // Ignore conflicting evidence, but treat it as an error
-              // because it should never happen and needs to be prevented
-              // from happening again.
-              trace(
-                '🚨 conflicting evidence for',
-                txHash,
-                ':',
-                lastEvidence,
-                '!=',
-                next,
-              );
-              Fail`conflicting evidence for ${quote(txHash)}`;
-            }
-            lastEvidence = next;
-          }
-
           const riskStores = [...risks.values()].filter(store =>
             store.has(txHash),
           );
-          // take the union of risks identified from all operators
-          const risksIdentified = allRisksIdentified(riskStores, txHash);
 
-          // sufficient agreement, so remove from pending risks, then publish
-          for (const store of found) {
-            store.delete(txHash);
+          // Publish at the threshold of agreement
+          if (found.length === minAttestations) {
+            // take the union of risks identified from all operators
+            const risksIdentified = allRisksIdentified(riskStores, txHash);
+            trace('publishing evidence', evidence, risksIdentified);
+            publisher.publish({
+              evidence,
+              risk: { risksIdentified },
+            });
+            return;
           }
-          for (const store of riskStores) {
-            store.delete(txHash);
+
+          if (found.length === pending.getSize()) {
+            // all have reported so clean up
+            for (const store of found) {
+              store.delete(txHash);
+            }
+            for (const store of riskStores) {
+              store.delete(txHash);
+            }
           }
-          trace('publishing evidence', evidence, risksIdentified);
-          publisher.publish({
-            evidence,
-            risk: { risksIdentified },
-          });
         },
       },
       public: {

diff --git a/packages/fast-usdc/src/fast-usdc.contract.js b/packages/fast-usdc/src/fast-usdc.contract.js
@@ -67,10 +67,11 @@ harden(meta);
  * @param {ERef<Marshaller>} marshaller
  * @param {FeeConfig} feeConfig
  */
-const publishFeeConfig = async (node, marshaller, feeConfig) => {
+const publishFeeConfig = (node, marshaller, feeConfig) => {
   const feeNode = E(node).makeChildNode(FEE_NODE);
-  const value = await E(marshaller).toCapData(feeConfig);
-  return E(feeNode).setValue(JSON.stringify(value));
+  void E.when(E(marshaller).toCapData(feeConfig), value =>
+    E(feeNode).setValue(JSON.stringify(value)),
+  );
 };
 
 /**
@@ -247,7 +248,7 @@ export const contract = async (zcf, privateArgs, zone, tools) => {
   // So we use zone.exoClassKit above to define the liquidity pool kind
   // and pass the shareMint into the maker / init function.
 
-  void publishFeeConfig(storageNode, marshaller, feeConfig);
+  publishFeeConfig(storageNode, marshaller, feeConfig);
 
   const shareMint = await provideSingleton(
     zone.mapStore('mint'),