feat(fast-usdc): settler disburses or forwards funds #10530

dckc · 2024-11-20T04:25:43Z

Description

feat(fast-usdc): settler disburses or forwards funds

Discussion of what to do if the minted USDC shows up at each state led to some refinement of states.
So the scope of this PR expanded somewhat:

chore(fast-usdc): status manager: split out Advancing state
chore(fast-usdc): advancer: split ADVANCING state

Security Considerations

In addition to the normal case where funds are repaid to the pool and fees are distributed, the settler is responsible to forward funds in case they were not advanced.

Scaling Considerations

nothing novel

Documentation Considerations

Readers are assumed to be familiar with design docs.

Testing Considerations

Getting the tests to pass requires:

fix(orchestration): subscribeToTransfers() atomically #10553

It's included in this PR for now but is expected to land separately

DRAFT until

test "Settlement for unknown transaction" case

Upgrade Considerations

This is a new component.

cloudflare-workers-and-pages · 2024-11-20T04:28:39Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`d67cd22`
Status:	✅ Deploy successful!
Preview URL:	https://12e7d7e6.agoric-sdk.pages.dev
Branch Preview URL:	https://10391-settler-exo.agoric-sdk.pages.dev

View logs

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

0xpatrickdev · 2024-11-22T02:53:29Z

Please see #10550 for advancer integration and status manager tests. It should be green minus the debugger; statements and unreachable code in monitorMintingDeposits. We might want to tackle monitorTransfers in a separate PR.

Edit: ea6b768 seems to be a temporary fix

turadg · 2024-11-22T21:30:45Z

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

-        }
+      disbursed(txHash, address, amount) {
+        // TODO: store txHash -> evidence for txs pending settlement?
+        console.log('TODO: vstorage update', { txHash, address, amount });


can wait for another PR but should at least have a TODO in this one…

define the states and valid transitions in constants, and when transitioning state verify that it's a valid transition. like in

agoric-sdk/packages/inter-protocol/src/vaultFactory/vault.js

Lines 67 to 77 in 2dc941f

/**

* @typedef {Exclude<Phase, 'transfer'>} VaultPhase

* @type {{ [K in VaultPhase]: VaultPhase[] }}

*/

const validTransitions = {

[Phase.ACTIVE]: [Phase.LIQUIDATING, Phase.CLOSED],

[Phase.LIQUIDATING]: [Phase.LIQUIDATED, Phase.ACTIVE],

[Phase.LIQUIDATED]: [Phase.CLOSED],

[Phase.CLOSED]: [],

};

I'm adding a TODO in constants.js, but in this disbursed(...) case, the previous state has been dequeued, so this exo no longer has it. In fact, I'm struggling to find anywhere that such a check would be relevant.

gotcha, maybe it doesn't make sense here with this virtual entity

turadg · 2024-11-22T21:31:41Z

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

+        recordPendingTx(evidence, PendingTxStatus.Advancing);
+      },
+
+      /**


please some more docs on when this is to be called

turadg · 2024-11-22T21:36:37Z

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

@@ -109,8 +132,34 @@ export const prepareStatusManager = zone => {
       * Add a new transaction with ADVANCED status
       * @param {CctpTxEvidence} evidence
       */
-      advance(evidence) {
-        recordPendingTx(evidence, PendingTxStatus.Advanced);
+      advancing(evidence) {


I think advance was correct. That's the transition event that will change the state to Advancing.

Consider having a naming scheme for all transition events. "send" is common in state FSMs. So here could be sendAdvance

restored advance

just added a TODO re naming scheme

turadg · 2024-11-22T21:38:07Z

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

+       * @param {boolean} success
+       * @throws {Error} if nothing to advance
+       */
+      advanceOutcome(sender, amount, success) {


it makes sense that this takes a boolean but for the calling code consider having a more explicit sendAdvanceSuccess and sendAdvanceFailure (which could call this)

I'm struggling with this "sendX" phrasing. We're telling the status manager to send something?

How about settleX? To signify the transfer vow settlement.

I don't mind the Boolean. Another option is a Fulfilled | Rejected const that would be more explicit at the call site.

Also keep in mind we'll soon need settleForward (in addition to settleAdvance or whatever is decided here)

"send" phrasing is from the metaphor of sending a message to the state machine. It's in a bunch of state machine libraries.

my comment was just a "consider" which is done.

turadg · 2024-11-22T21:39:19Z

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

-       * Find an `ADVANCED` or `OBSERVED` tx waiting to be `SETTLED`
+       * @param {CctpTxEvidence} evidence
+       */
+      isSeen(evidence) {


is connotes a check on the current state but Seen isn't one of the states.

hasBeenObserved?

turadg · 2024-11-22T21:40:18Z

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

      },

      /**
       * Lookup all pending entries for a given address and amount
       *
+       * XXX only used in tests. should we remove?


yes. tests need only cover what can be called in production.

I wrote this comment - some tests would need to be reworked if we were to remove this. They use this method to confirm state transitions.

I wonder if they need to do so explicitly. Kind of testing the implementation.

Maybe a better approach is to define the stores as optional initState arguments so we can access the contents in tests.

cleaner wrt the API surface but that's even moreso testing the implementation

0xpatrickdev

Nice job. My review is partial since we paired and tag teamed much of this. I think it's good to land but noted some additional requirements that could be part of #10391 or a follow up ticket.

0xpatrickdev · 2024-11-22T21:31:26Z

packages/fast-usdc/src/constants.js

+  /** fallback: do not collect fees */
+  Forwarded: 'FORWARDED',
+  /** failed to forward to EUD */
+  ForwardFailed: 'FORWARD_FAILED',


Suggested change

ForwardFailed: 'FORWARD_FAILED',

ForwardFailed: 'FORWARD_FAILED',

/** started IBC transfer for fallback path */

Forwarding: 'FORWARDING',

0xpatrickdev · 2024-11-22T21:34:08Z

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

+            dest,
+            AmountMath.make(USDC, amount),
+          );
+          await vowTools.when(txfrV); // TODO: watch, handle failure


This PR tackles a lot so I won't push to include here - but please create a follow up ticket or update this to refs that captures work needed for Forwarding and ForwardFailed states.

We don't in general have issues for all TODOs, do we? Is this one special?

0xpatrickdev · 2024-11-22T21:36:51Z

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

+        }
+
+        const { status, txHash } = pending[dequeueIdx];
+        // TODO: store txHash -> evidence for txs pending settlement?


Suggested change

// TODO: store txHash -> evidence for txs pending settlement?

// TODO: store txHash -> evidence for txs pending settlement?

// If necessary for vstorage writes in `forwarded` and `settled`

0xpatrickdev · 2024-11-22T21:37:28Z

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

       * @param {NobleAddress} address
       * @param {bigint} amount
       * @returns {PendingTx[]}
       */
      lookupPending(address, amount) {
        const key = makePendingTxKey(address, amount);
        if (!pendingTxs.has(key)) {
-          throw makeError(`Key ${q(key)} not yet observed`);
+          return [];


Suggested change

return [];

return harden([]);

0xpatrickdev · 2024-11-22T21:40:29Z

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

+            // TODO #10390 log all early returns
+            // only interested in packets from the issuing chain
+            return;


Before closing #10390, please log all early returns. This was a previous request: https://github.com/Agoric/agoric-sdk/pull/10406/files#r1831815520

0xpatrickdev · 2024-11-22T22:02:56Z

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

        const pending = pendingTxs.get(key);
-        return !!pending.length;
+        if (!pending.length) return undefined;


Suggested change

if (!pending.length) return undefined;

We might be able to remove this - it seems unreachable with the pendingTxs.delete logic in dequeueStatus. (Also the only line popping up on test:c8)

it's also taken care of by the dequeueIdx < 0 check after find. it's at best an optimization.

turadg

LGTM.

dckc · 2024-11-25T05:55:47Z

I'm struggling with this ci mystery:

 Error {
    message: 'tryExitOffer failed to reclaim stuck payment  condition failed after 15 retries.',

I re-ran it; it still failed.

Jorge-Lopes · 2024-11-25T18:43:18Z

 Error {
    message: 'tryExitOffer failed to reclaim stuck payment  condition failed after 15 retries.',

It’s strange—I can see in the logs the message:
wallet agoric1ee9hr0jyrxhy999y755mp862ljgycmwyp4pl7q reclaimed [ { brand: Object [Alleged: IST brand] {}, value: 15_000n } ] from bad-invitation-15,
which indicates that the stuck payment was reclaimed and the wallet’s IST balance should have increased and satisfied the expected condition.

However, since the istBalanceBefore value isn’t being logged, we cannot properly verify this.
Here’s the link to the relevant logs:
GitHub Action Logs.

What I don’t understand is why this test doesn’t fail when running the a3p-integration acceptance tests locally or in other PRs (for example here, where the acceptance wallet test is passing), but it’s failing here—even though I don’t see any changes being introduced that to a3p-integration proposals.

Am I failing to notice any change being introduced in this PR that may conflict with the any a3p-integration acceptance test?

dckc · 2024-11-25T19:05:44Z

Am I failing to notice any change being introduced in this PR that may conflict with the any a3p-integration acceptance test?

I can't imagine any. All the changes are in the fast-usdc package, which isn't even deployed in the z:acceptance context.
With 1 exception: a tweak in orchestration. I can't imagine it's causing problems, but I don't suppose I can say for certain that it's not.

dckc · 2024-11-25T19:06:52Z

I see some earlier symptoms, at 4370:

    Error: error in json rpc client, with http response metadata: (Status: 200 OK, Protocol HTTP/1.1). RPC error -32603 - Internal error: timed out waiting for tx to be included in a block

- settle -> disburse

settler, advancer

dckc · 2024-11-25T19:55:20Z

rebased now that #10553 has landed

turadg · 2024-11-25T21:44:40Z

I'm bypassing integration because this is passing every integration test but this flake,

"exitOffer tool reclaims stuck payment" timed out waiting for tx to be included in a block #10565

refs: #10565 ## Description #10530 was failing repeatedly due to this test problem, - #10565 I thought it was a flake so I [bypassed the check](#10530 (comment)) but it turns out to fail deterministically due to the changes in #10530. But those changes should have not affected the test, so the problem is with the test. This marks it as failing until it can be solved. ### Security Considerations none ### Scaling Considerations none ### Documentation Considerations none ### Testing Considerations I used `failing` instead of `skip` to help confirm that it's deterministic. Also if something happens to resolve it inadvertently it'll be detected so we re-enable it. ### Upgrade Considerations none

closes: #10565 refs: #10774 ## Description Test named `exitOffer tool reclaims stuck payment` was flakey and this prevented landing other PRs like #10530. ### Security Considerations None. ### Scaling Considerations None. ### Documentation Considerations None. ### Testing Considerations The flakey test was being skipped before. Now we included it back in and the CI should pass. ### Upgrade Considerations This test concerns current and all future upgrades as the acceptance should pass before landing upgrades.

0xpatrickdev reviewed Nov 20, 2024

View reviewed changes

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

dckc force-pushed the 10391-settler-exo branch 3 times, most recently from 9e90738 to 09171cb Compare November 21, 2024 21:53

dckc force-pushed the 10391-settler-exo branch from 9e5c099 to ab020c2 Compare November 22, 2024 17:13

mergify bot mentioned this pull request Nov 22, 2024

feat: refine status manager #10550

Merged

dckc force-pushed the 10391-settler-exo branch 3 times, most recently from 50f3674 to 2dc941f Compare November 22, 2024 20:53

dckc changed the title ~~10391 settler exo~~ feat(fast-usdc): settler disburses or forwards funds Nov 22, 2024

dckc requested review from turadg and 0xpatrickdev November 22, 2024 20:54

dckc marked this pull request as ready for review November 22, 2024 21:00

dckc requested a review from a team as a code owner November 22, 2024 21:00

turadg reviewed Nov 22, 2024

View reviewed changes

0xpatrickdev reviewed Nov 22, 2024

View reviewed changes

0xpatrickdev requested review from 0xpatrickdev and removed request for 0xpatrickdev November 22, 2024 21:42

0xpatrickdev reviewed Nov 22, 2024

View reviewed changes

turadg approved these changes Nov 23, 2024

View reviewed changes

dckc force-pushed the 10391-settler-exo branch from cb93488 to e4bf666 Compare November 23, 2024 00:23

dckc added the automerge:rebase Automatically rebase updates, then merge label Nov 23, 2024

dckc force-pushed the 10391-settler-exo branch from e4bf666 to 0ab9e37 Compare November 23, 2024 00:43

turadg added this to the FU1: package integration milestone Nov 25, 2024

dckc removed the automerge:rebase Automatically rebase updates, then merge label Nov 25, 2024

dckc mentioned this pull request Nov 25, 2024

"exitOffer tool reclaims stuck payment" timed out waiting for tx to be included in a block #10565

Closed

samsiegart mentioned this pull request Nov 25, 2024

feat(fast-usdc): write status updates to vstorage #10552

Merged

dckc and others added 7 commits November 25, 2024 13:54

refactor(fast-usdc): export LiquidityPoolKit from types.ts

e58d821

docs: move status manager state diagrams

745d399

chore(fast-usdc): status manager: split out Advancing state

b63bd6f

- settle -> disburse

chore(fast-usdc): advancer: split ADVANCING state

9a652f4

feat(fast-usdc): settler disburses or forwards funds

17b0423

chore(fast-usdc): update contract for exo APIs

d060a25

settler, advancer

chore(fast-usdc): update test snapshots

d67cd22

dckc force-pushed the 10391-settler-exo branch from 0ab9e37 to d67cd22 Compare November 25, 2024 19:54

dckc added the automerge:rebase Automatically rebase updates, then merge label Nov 25, 2024

turadg added the bypass:integration Prevent integration tests from running on PR label Nov 25, 2024

mergify bot merged commit aa11d5c into master Nov 25, 2024
99 of 101 checks passed

mergify bot deleted the 10391-settler-exo branch November 25, 2024 21:44

turadg mentioned this pull request Nov 26, 2024

test: reclaim stuck payment failing #10569

Merged

0xpatrickdev mentioned this pull request Nov 26, 2024

Comprehensive testing of failure scenarios #10510

Closed

anilhelvaci mentioned this pull request Dec 26, 2024

fix(acceptance): fix flakey wallet test #10775

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(fast-usdc): settler disburses or forwards funds #10530

feat(fast-usdc): settler disburses or forwards funds #10530

dckc commented Nov 20, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Nov 20, 2024 •

edited

Loading

0xpatrickdev commented Nov 22, 2024 •

edited

Loading

turadg Nov 22, 2024

dckc Nov 22, 2024

turadg Nov 22, 2024

turadg Nov 22, 2024

turadg Nov 22, 2024

dckc Nov 22, 2024

turadg Nov 22, 2024

dckc Nov 22, 2024

0xpatrickdev Nov 22, 2024

turadg Nov 22, 2024

turadg Nov 22, 2024

turadg Nov 22, 2024

0xpatrickdev Nov 22, 2024

turadg Nov 22, 2024

0xpatrickdev Nov 22, 2024

turadg Nov 23, 2024 •

edited

Loading

0xpatrickdev left a comment

0xpatrickdev Nov 22, 2024

0xpatrickdev Nov 22, 2024

dckc Nov 22, 2024

0xpatrickdev Nov 22, 2024

0xpatrickdev Nov 22, 2024

0xpatrickdev Nov 22, 2024

0xpatrickdev Nov 22, 2024

dckc Nov 22, 2024

turadg left a comment

dckc commented Nov 25, 2024

Jorge-Lopes commented Nov 25, 2024 •

edited

Loading

dckc commented Nov 25, 2024

dckc commented Nov 25, 2024

dckc commented Nov 25, 2024

turadg commented Nov 25, 2024

	/**
	* @typedef {Exclude<Phase, 'transfer'>} VaultPhase
	* @type {{ [K in VaultPhase]: VaultPhase[] }}
	*/
	const validTransitions = {
	[Phase.ACTIVE]: [Phase.LIQUIDATING, Phase.CLOSED],
	[Phase.LIQUIDATING]: [Phase.LIQUIDATED, Phase.ACTIVE],
	[Phase.LIQUIDATED]: [Phase.CLOSED],
	[Phase.CLOSED]: [],
	};

	// TODO: store txHash -> evidence for txs pending settlement?
	// TODO: store txHash -> evidence for txs pending settlement?
	// If necessary for vstorage writes in `forwarded` and `settled`

feat(fast-usdc): settler disburses or forwards funds #10530

feat(fast-usdc): settler disburses or forwards funds #10530

Conversation

dckc commented Nov 20, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Nov 20, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

0xpatrickdev commented Nov 22, 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

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 Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

0xpatrickdev 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

turadg left a comment

Choose a reason for hiding this comment

dckc commented Nov 25, 2024

Jorge-Lopes commented Nov 25, 2024 • edited Loading

dckc commented Nov 25, 2024

dckc commented Nov 25, 2024

dckc commented Nov 25, 2024

turadg commented Nov 25, 2024

dckc commented Nov 20, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Nov 20, 2024 •

edited

Loading

0xpatrickdev commented Nov 22, 2024 •

edited

Loading

turadg Nov 23, 2024 •

edited

Loading

Jorge-Lopes commented Nov 25, 2024 •

edited

Loading