chore(fusdc): settle `ForwardFailed`, minted while `Advancing`, and minted early txs #10729

0xpatrickdev · 2024-12-18T15:46:19Z

closes: #10625

Description

fix: ensure "minted while Advancing" results in Disburse or Forward
feat: handle "unknown mint" by forwarding if and when Evidence is reported
- via Settler.forwardIfMinted() called by Advancer
chore: capture ForwardFailed (terminal) state if Forward fails

Security Considerations

None new introduced, these behaviors are consistent with the product spec.

Scaling Considerations

The mintedEarly mapStore could grow large if an attacker spams the settlementAccount with uusdc

Documentation Considerations

None

Testing Considerations

Includes tests for all new behaviors.

Upgrade Considerations

Targeting FUSDC's first release

cloudflare-workers-and-pages · 2024-12-18T15:49:38Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`9c01dc4`
Status:	✅ Deploy successful!
Preview URL:	https://a4046484.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-10625-forward-failed.agoric-sdk.pages.dev

View logs

dckc

a couple notes before our chat

dckc · 2024-12-18T16:22:16Z

packages/fast-usdc/src/exos/advancer.js

+            } catch (e) {
+              // settlement already received; tx will Forward.


assuming all exceptions have this meaning seems inconsistent with https://github.com/Agoric/agoric-sdk/wiki/Errors-and-Control-Flow

packages/fast-usdc/src/exos/settler.js

0xpatrickdev · 2024-12-19T21:51:12Z

packages/fast-usdc/src/exos/settler.js

+              log('⚠️ tap: minted before observed', nfa, amount);
+              // XXX consider capturing in vstorage
+              // we would need a new key, as this does not have a txHash
+              this.state.mintedEarly.add(makeMintedEarlyKey(nfa, amount));


I realized while writing this PR desc:

The mintedEarly mapStore could grow large if an attacker spams the settlementAccount with uusdc

Worth adding a check here to see if the advance exceeds min fees? That should like help with 1uusdc spam.

0xpatrickdev · 2024-12-19T21:57:20Z

packages/fast-usdc/src/exos/settler.js

@@ -206,19 +215,19 @@ export const prepareSettler = (
        ) {
          const { mintedEarly } = this.state;
          const { value: fullValue } = fullAmount;
+          // XXX i think this only contains Advancing txs - should this be a separate store?


comment for reviewers, not something to check in

what's the alternative? pros/cons?

It'd take some time to think of an attack vector - currently coming up short.

In general, they're pretty distinct behaviors / states so commingling didn't seem right.

Since we don't currently have sufficient motivation to separate, I'll leave as-is for now and remove the comment.

0xpatrickdev · 2024-12-19T22:01:06Z

packages/fast-usdc/test/fast-usdc.contract.test.ts

+  t.deepEqual(bridgeTraffic.local, [], 'no IBC transfers');
+
+  // activate oracles and submit evidence; expect Settler to forward (slow path)
+  // 'C12 - Contract MUST only pay back the Pool (fees) only if they started the advance before USDC is minted',


This is maybe not entirely relevant. We could adjust the test to fail (AdvanceFailed), and we would not pay fees to the pool here. This is covered in settler.test.ts, though.

0xpatrickdev · 2024-12-19T22:23:29Z

packages/fast-usdc/test/fast-usdc.contract.test.ts

+  const forwardInfo = JSON.parse(outgoingForwardMessage.memo).forward;
+  t.is(forwardInfo.receiver, EUD, 'receiver is osmo10tt0');
+
+  // in lieu of transmitTransferAck so we can set a nonce that matches our initial Advance


considered parameterizing like: transmitTransferAck(-1) but came up short. I think it's only the sequence that difference, not the index of localBridgeMessage.

dckc

here are some comments based on one read / skim

dckc · 2024-12-19T22:08:07Z

packages/fast-usdc/src/exos/settler.js

-          log('⚠️ transfer rejected!', reason, ctx);
-          // const { txHash, nfa, amount } = ctx;
+        onRejected(reason, txHash) {
+          log('⚠️ transfer rejected!', reason, txHash);
          // TODO(#10510): statusManager.forwardFailed(txHash, nfa, amount);


#10510 is closed. should this TODO still be here?

I think this was removed in a later commit but will check again

dckc · 2024-12-19T22:09:42Z

packages/fast-usdc/src/exos/status-manager.js

+    // eslint-disable-next-line no-unused-vars
    log = makeTracer('Advancer', true),


this tracer shouldn't call itself 'Advancer'.
out of scope of this PR, but since you're in the neighborhood...

dckc · 2024-12-19T22:10:00Z

packages/fast-usdc/src/exos/status-manager.js

@@ -69,6 +69,7 @@ export const prepareStatusManager = (
  txnsNode,
  {
    marshaller,
+    // eslint-disable-next-line no-unused-vars


we should use the log/tracer, shouldn't we?

There's nothing currently logging anything in statusManager. vstorage seems to be the main way of communicating

dckc · 2024-12-19T22:11:00Z

packages/fast-usdc/src/exos/status-manager.js

-       * @param {EvmHash | undefined} txHash - undefined in case mint before observed
-       * @param {NobleAddress} nfa
-       * @param {bigint} amount
+       * @param {EvmHash} txHash - undefined in case mint before observed


the "undefined in case..." docstring looks outdated

I suppose the preconditions for forwarded(...) are in a state transition diagram somewhere, but it would be nice to have them here locally.

In particular, this would throw if you call it twice on the same txHash, right?

dckc · 2024-12-19T22:17:56Z

packages/fast-usdc/src/exos/settler.js

          const key = makeMintedEarlyKey(forwardingAddress, fullValue);
          if (mintedEarly.has(key)) {
            mintedEarly.delete(key);
            if (success) {
+              // TODO: does not write `ADVANCED` to vstorage


TODO when? in this PR?

#10750 but maybe will collapse into this

Closing 10750 and collapsing into this PR

dckc · 2024-12-19T22:18:16Z

packages/fast-usdc/src/exos/status-manager.js

-            status: M.or(
-              PendingTxStatus.Advanced,
-              PendingTxStatus.AdvanceFailed,
-              PendingTxStatus.Observed,
-            ),
+            status: M.or(...Object.values(PendingTxStatus)),


dckc · 2024-12-19T22:19:29Z

packages/fast-usdc/src/exos/status-manager.js

@@ -277,7 +273,10 @@ export const prepareStatusManager = (
      },

      /**
-       * Remove and return an `ADVANCED` or `OBSERVED` tx waiting to be `SETTLED`.
+       * Remove and return the _first_ `PendingSettleTx` that matches the observed
+       * settlement. This is not necessarily an exact match with the original


what does "not necessarily an exact match" mean? isn't "same forwarding account and amount" the relevant definition of "matching"?

The ordering could be different if Alice requests two advances for the same amount.

It is an exact match based on available data, which treats (EUD,Amount) interchangeably.

I think think what you're getting at is that a mint deposit may be for any of the matching transactions and we can't distinguish between them, so we go with the first.

How's this:

/** * Remove and return the oldest pending settlement transaction that matches the given * forwarding account and amount. Since multiple pending transactions may exist with * identical (account, amount) pairs, we process them in FIFO order. */

dckc · 2024-12-19T22:29:03Z

packages/fast-usdc/test/exos/settler.test.ts

+  ]);
+});
+
+test.todo('create facet methods');


creator facet?

packages/fast-usdc/src/exos/settler.js

turadg · 2024-12-19T23:24:31Z

packages/fast-usdc/src/exos/advancer.js

+              txHash,
+            } = evidence;
+
+            const { borrowerFacet, notifyFacet, poolAccount } = this.state;


sometime before this PR, but I'm surprised to see Facet in the name of the object. the consumer needn't be concerned with the structure in which the object resides.

I've added this to revisit in #10432

turadg · 2024-12-19T23:27:14Z

packages/fast-usdc/src/exos/advancer.js

+            if (
+              notifyFacet.forwardIfMinted(
+                destination,
+                forwardingAddress,
+                fullAmount,
+                txHash,
+              )
+            ) {


surprising to see the condition be the result of an action. please make more clear.

Suggested change

if (

notifyFacet.forwardIfMinted(

destination,

forwardingAddress,

fullAmount,

txHash,

)

) {

const forwarded = notifyFacet.forwardIfMinted(

destination,

forwardingAddress,

fullAmount,

txHash,

);

if (forwarded)) {

This name was weird to me and as I get through more of the review, I understand better why. The notify facet shouldn't be telling the settler what to do.

What is it notifying it of? It's telling the settler about a transaction and it needs to know what decision the settler has to say about it. So I think this should be,

Suggested change

if (

notifyFacet.forwardIfMinted(

destination,

forwardingAddress,

fullAmount,

txHash,

)

) {

const forwarded = notifyFacet.checkForwarded(

destination,

forwardingAddress,

fullAmount,

txHash,

);

if (forwarded) {

return;

}

I took out the logging too because the settler already does that. I'm not as confident in that change.

This was tricky to name since "checkForwarded" does more than checking - it actually starts a Forward via MsgTransfer if conditions are met. That's why i wanted to include a verb.

I've heeded your suggestion, and renamed to checkIfMinted. (the forward hasn't occurred yet, and the early mint is the thing we're checking)

it actually starts a Forward via MsgTransfer if conditions are met. That's why i wanted to include a verb.

It happens to but needn't be a commitment to the caller. The caller is providing information and asking for a state.

turadg · 2024-12-19T23:32:15Z

packages/fast-usdc/src/exos/status-manager.js

@@ -277,7 +273,10 @@ export const prepareStatusManager = (
      },

      /**
-       * Remove and return an `ADVANCED` or `OBSERVED` tx waiting to be `SETTLED`.
+       * Remove and return the _first_ `PendingSettleTx` that matches the observed
+       * settlement. This is not necessarily an exact match with the original


It is an exact match based on available data, which treats (EUD,Amount) interchangeably.

I think think what you're getting at is that a mint deposit may be for any of the matching transactions and we can't distinguish between them, so we go with the first.

turadg · 2024-12-19T23:33:30Z

packages/fast-usdc/src/exos/advancer.js

+                txHash,
+              )
+            ) {
+              // settlement already received; tx will Forward.


It's not "will Forward" because that has started at this point. I don't think this comment adds enough over the log message and the if (forwarded) branch

Suggested change

// settlement already received; tx will Forward.

packages/fast-usdc/src/exos/advancer.js

packages/fast-usdc/src/exos/settler.js

turadg · 2024-12-19T23:40:48Z

packages/fast-usdc/src/exos/settler.js

@@ -206,19 +215,19 @@ export const prepareSettler = (
        ) {
          const { mintedEarly } = this.state;
          const { value: fullValue } = fullAmount;
+          // XXX i think this only contains Advancing txs - should this be a separate store?


what's the alternative? pros/cons?

turadg · 2024-12-19T23:41:14Z

packages/fast-usdc/src/exos/settler.js

            } else {
+              // TODO: does not write `ADVANCE_FAILED` to vstorage


Suggested change

// TODO: does not write `ADVANCE_FAILED` to vstorage

// FIXME: does not write `ADVANCE_FAILED` to vstorage

TODO will appear in commit history, but not the final HEAD of this PR

packages/fast-usdc/src/exos/status-manager.js

0xpatrickdev · 2024-12-20T17:29:09Z

packages/fast-usdc/src/exos/status-manager.js

+       * @param {EvmHash} txHash
+       * @param {boolean} success whether the Transfer succeeded
+       */
+      advanceOutcomeForSettled(txHash, success) {


Not crazy about advanceOutcomeForSettled, but implemented the suggestion.

I liked notify since all we're doing here is a vstorage write. Technically, it's not settled yet (we could say minted).

Where does that leave us with notifyObserved?

I liked notify since all we're doing here is a vstorage write.

That shouldn't be a commitment to the caller. The caller tells the status manager about stuff and the status manager abstracts what it does with that information.

Technically, it's not settled yet (we could say minted).

Yeah that would be better.

Where does that leave us with notifyObserved?

To be renamed for the same reasons

I landed on advanceOutcomeForMintedEarly and advanceOutcomeForUnknownMint. If we want something more concise, i think we can tackle that in #10432

- verify poolAccount is sender for Advances - verify Advance is Disbursed in happy path - verify pool and settlement account addresses (better visibility in active dev)

- Advancing txs are dequeueable so `case PendingTxStatus.Advancing:` is reachable - update Settler tests to use `notifyAdvancing` result to simulate an advance - add Settler test for minting while advancing (Advanced + AdvanceFailed paths)

- if Forward (slow transfer) fails, capture the state to distinguish from successful forwards

- Advancer calls `forwardIfMinted` to check for an early matching mint - if found, no Advance occurs and the minted funds are forwarded

- ensure "minted before observed" `Observe` statuses are recorded in vstorage - ensure "minted while Advancing" `Advanced` and `AdvanceFailed` statuses are recorded in vstorage

dckc reviewed Dec 18, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/10625-forward-failed branch 5 times, most recently from 0c54b6f to 443b5cc Compare December 19, 2024 21:44

0xpatrickdev commented Dec 19, 2024

View reviewed changes

0xpatrickdev added the force:integration Force integration tests to run on PR label Dec 19, 2024

0xpatrickdev marked this pull request as ready for review December 19, 2024 21:51

0xpatrickdev requested a review from a team as a code owner December 19, 2024 21:51

0xpatrickdev requested review from AgoricTriage, turadg and dckc December 19, 2024 21:51

0xpatrickdev commented Dec 19, 2024

View reviewed changes

dckc reviewed Dec 19, 2024

View reviewed changes

0xpatrickdev commented Dec 19, 2024

View reviewed changes

packages/fast-usdc/src/exos/settler.js Outdated Show resolved Hide resolved

turadg reviewed Dec 19, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/10625-forward-failed branch 2 times, most recently from 08bf844 to 3a0afe7 Compare December 20, 2024 17:27

0xpatrickdev commented Dec 20, 2024

View reviewed changes

0xpatrickdev mentioned this pull request Dec 20, 2024

chore: tx statuses in vstorage #10750

Closed

0xpatrickdev force-pushed the pc/10625-forward-failed branch from 3a0afe7 to 3e111dc Compare December 20, 2024 18:07

0xpatrickdev added 5 commits December 20, 2024 15:06

chore: pendingTxs -> pendingSettleTxs

4d4fb9c

chore: misc fusdc contract test improvements

4b86ee9

- verify poolAccount is sender for Advances - verify Advance is Disbursed in happy path - verify pool and settlement account addresses (better visibility in active dev)

chore: only Observed txs are Forwardable

68ca24f

test: mint received while ADVANCING

ba4a53a

fix: Advancing txs are dequeueable

e8776fa

- Advancing txs are dequeueable so `case PendingTxStatus.Advancing:` is reachable - update Settler tests to use `notifyAdvancing` result to simulate an advance - add Settler test for minting while advancing (Advanced + AdvanceFailed paths)

0xpatrickdev added 2 commits December 20, 2024 15:24

chore: capture ForwardFailed state

4c5c6f0

- if Forward (slow transfer) fails, capture the state to distinguish from successful forwards

feat: evidence after mint results in forward

f8d5b33

- Advancer calls `forwardIfMinted` to check for an early matching mint - if found, no Advance occurs and the minted funds are forwarded

0xpatrickdev force-pushed the pc/10625-forward-failed branch from 3e111dc to 9367164 Compare December 20, 2024 20:25

0xpatrickdev requested review from turadg and dckc December 20, 2024 20:28

chore: tx statuses in vstorage

9c01dc4

- ensure "minted before observed" `Observe` statuses are recorded in vstorage - ensure "minted while Advancing" `Advanced` and `AdvanceFailed` statuses are recorded in vstorage

0xpatrickdev force-pushed the pc/10625-forward-failed branch from 9367164 to 9c01dc4 Compare December 20, 2024 20:33

0xpatrickdev changed the title ~~chore(fusdc): settle ForwardFailed and Advancing txs~~ chore(fusdc): settle ForwardFailed, minted while Advancing, and minted early txs Dec 20, 2024

		} catch (e) {
		// settlement already received; tx will Forward.

		// eslint-disable-next-line no-unused-vars
		log = makeTracer('Advancer', true),

		} else {
		// TODO: does not write `ADVANCE_FAILED` to vstorage

	// TODO: does not write `ADVANCE_FAILED` to vstorage
	// FIXME: does not write `ADVANCE_FAILED` to vstorage

chore(fusdc): settle ForwardFailed, minted while Advancing, and minted early txs #10729

Are you sure you want to change the base?

chore(fusdc): settle ForwardFailed, minted while Advancing, and minted early txs #10729

Conversation

0xpatrickdev commented Dec 18, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Dec 18, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

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

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

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

turadg Dec 20, 2024 • 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

chore(fusdc): settle `ForwardFailed`, minted while `Advancing`, and minted early txs #10729

chore(fusdc): settle `ForwardFailed`, minted while `Advancing`, and minted early txs #10729

0xpatrickdev commented Dec 18, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 18, 2024 •

edited

Loading

turadg Dec 20, 2024 •

edited

Loading