fix(fast-usdc): FORWARD_FAILED when EUD invalid #10963

samsiegart · 2025-02-07T05:14:30Z

Description

Adds multichain-testing scenarios for:

insufficient LP funds; forward path
minted before observed; forward path
insufficient LP funds and forward failed

Changed the behavior of the settler to go to forwardFailed state if it tries to forward to an invalid/un-registered chain. The previous behavior was to get stuck at observed.

Also added a test advance failed (e.g. to missing chain) which is currently skipped because in the missing chain case, it actually just stays at the observed state. Should we change this to advanceFailed/forwardFailed?

Testing Considerations

Did this in multichain-testing rather than bootstrap because it's closer to a real environment. I previously observed the stuck-at-observed behavior when attempting to write a bootstrap test but couldn't figure out if it was an issue with my test logic.

0xpatrickdev

Nice job

0xpatrickdev · 2025-02-07T17:03:08Z

multichain-testing/test/fast-usdc/fast-usdc.test.ts

+  // XXX Not sure if the behavior should actually be ADVANCE_FAILED.
+  // It only reaches that state if the destination chain is valid.
+  await assertTxStatus(evidence.txHash, 'OBSERVED');


This is expected with the current implementation* - we'll only see AdvanceFailed if an advance was actually attempted. Here it seems chainHub.makeChainAddress, a precondition step, is failing since the ChainInfo / corresponding bech32 prefix is not present.

An approach to get AdvanceFailed in the test would be to add fake cosmos chain info (by extending commonBuilderOpts.chainInfo. If the connections or asset info are missing, this should trigger the transferHandler.onRejected block (AdvanceFailed). If that info is present but the connection/channel doesn't exist in the environment, it should also trigger a failure from golang.

*I wonder if it's worth using .skipAdvance instead of .observe in the advancer's catch block when pre-condition checks fail. We could serialize the error to vstorage for observability.

Just added the chain info and asserted ADVANCE_FAILED now that #10980 is merged, it worked as expected.

I wonder if it's worth using .skipAdvance instead of .observe in the advancer's catch block when pre-condition checks fail. We could serialize the error to vstorage for observability.

I'm impartial to that. I think it ends up forward failed either way right? But we'd find out sooner I guess if we serialize the error. Yea maybe it's better. Can that be a follow-up on this PR?

Can that be a follow-up on this PR?

Definitely, we should create a ticket if we think it's worth it. The main advantage I see is error/pre-condition observability via vstorage instead of swingset logs.

Created #10994

multichain-testing/test/fast-usdc/fast-usdc.test.ts

0xpatrickdev · 2025-02-07T17:25:47Z

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

+          let dest;
+          try {
+            dest = chainHub.makeChainAddress(EUD);
+          } catch (e) {
+            log('⚠️ forward transfer failed!', e, txHash);
+            return statusManager.forwarded(txHash, false);
+          }


The mutable dest is a smell.

Not a blocker for this testing PR, but I think we need a more general error handling solution.

audit and normalize error handling #10968

Also not a blocker but this change should not be part of a test commit. It's a runtime change, specifically a fix.

previously observed the stuck-at-observed behavior when attempting to write a bootstrap test

Was that bootstrap test hurdle due to the problem here? I.e. could we port these multichain-testing tests to bootstrap style and they'll work?

We might want to do that, to help uncover where errors are not handled correctly (eg where the test can't be written, like you ran into)

Could we port these multichain-testing tests to bootstrap style and they'll work?

I'm not sure unless I try. Maybe I can follow up after this PR?

If that's the case let's wait until #10811

cloudflare-workers-and-pages · 2025-02-08T07:41:09Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`cac5b62`
Status:	✅ Deploy successful!
Preview URL:	https://922d2532.agoric-sdk.pages.dev
Branch Preview URL:	https://test-forward-fail.agoric-sdk.pages.dev

View logs

turadg · 2025-02-11T17:37:12Z

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

+          let dest;
+          try {
+            dest = chainHub.makeChainAddress(EUD);
+          } catch (e) {
+            log('⚠️ forward transfer failed!', e, txHash);
+            return statusManager.forwarded(txHash, false);
+          }


statusManager.forwarded is void so we don't need to return it. Let's keep the const. One way:

Suggested change

let dest;

try {

dest = chainHub.makeChainAddress(EUD);

} catch (e) {

log('⚠️ forward transfer failed!', e, txHash);

return statusManager.forwarded(txHash, false);

}

const dest = (() => {

try {

return chainHub.makeChainAddress(EUD);

} catch (e) {

log('⚠️ forward transfer failed!', e, txHash);

statusManager.forwarded(txHash, false);

return null;

}});

if (!dest) return;

Though actually it looks like the log message should apply if any of this function threw, so best to wrap the whole thing in this try/catch.

Separately, this change is appearing in a test commit but it changes the runtime behavior. It's a fix or feat.

Changed to use a const instead.

I'm not sure about wrapping the whole thing in try-catch. The remaining lines are for getting the transfer vow and watching the result, where transferHandler should handle the error. If transferHandler throws, it might inadvertently call statusManager.forwarded again if so.

The commit is already a fix, I hadn't updated the PR description though--just did so.

turadg · 2025-02-11T17:38:39Z

multichain-testing/tools/query.ts

+    const maxRetries = 5;
+    const retryDelayMS = 500;


don't we have a general "retry promise up to N times with M delay" utility? cc @dckc

We have retryUntilCondition but there's no condition to check here, just that the request worked, so it felt strange to use here. This is just to stop the test from flaking when the RPC connection fails which I observed a few times.

That the request worked sounds like a condition. If it's hard to use that function for this case, maybe we could use another. Food for thought.

samsiegart

PTAL

samsiegart · 2025-02-12T09:44:44Z

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

+          let dest;
+          try {
+            dest = chainHub.makeChainAddress(EUD);
+          } catch (e) {
+            log('⚠️ forward transfer failed!', e, txHash);
+            return statusManager.forwarded(txHash, false);
+          }


Changed to use a const instead.

I'm not sure about wrapping the whole thing in try-catch. The remaining lines are for getting the transfer vow and watching the result, where transferHandler should handle the error. If transferHandler throws, it might inadvertently call statusManager.forwarded again if so.

The commit is already a fix, I hadn't updated the PR description though--just did so.

samsiegart · 2025-02-12T09:47:26Z

multichain-testing/tools/query.ts

+    const maxRetries = 5;
+    const retryDelayMS = 500;


We have retryUntilCondition but there's no condition to check here, just that the request worked, so it felt strange to use here. This is just to stop the test from flaking when the RPC connection fails which I observed a few times.

samsiegart requested a review from a team as a code owner February 7, 2025 05:14

samsiegart requested review from AgoricTriage, turadg and 0xpatrickdev February 7, 2025 05:14

0xpatrickdev approved these changes Feb 7, 2025

View reviewed changes

samsiegart force-pushed the test-forward-fail branch 2 times, most recently from 616fab6 to a1f13a5 Compare February 11, 2025 04:39

samsiegart requested a review from 0xpatrickdev February 11, 2025 04:59

turadg mentioned this pull request Feb 11, 2025

move Fast USDC into its own repo #10811

Open

turadg reviewed Feb 11, 2025

View reviewed changes

samsiegart changed the title ~~test(fast-usdc): multichain test for forward~~ fix(fast-usdc): FORWARD_FAILED when EUD invalid Feb 12, 2025

samsiegart commented Feb 12, 2025

View reviewed changes

samsiegart requested a review from turadg February 12, 2025 09:53

turadg approved these changes Feb 12, 2025

View reviewed changes

samsiegart mentioned this pull request Feb 12, 2025

Use skipAdvance rather than observe when precondition checks fail #10994

Open

samsiegart added the automerge:rebase Automatically rebase updates, then merge label Feb 12, 2025

samsiegart force-pushed the test-forward-fail branch from bbcb629 to b4fa9ce Compare February 12, 2025 19:34

fix(fast-usdc): FORWARD_FAILED when EUD invalid

cac5b62

samsiegart force-pushed the test-forward-fail branch from b4fa9ce to cac5b62 Compare February 12, 2025 20:19

mergify bot merged commit a67bf28 into master Feb 12, 2025
83 checks passed

mergify bot deleted the test-forward-fail branch February 12, 2025 21:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(fast-usdc): FORWARD_FAILED when EUD invalid #10963

fix(fast-usdc): FORWARD_FAILED when EUD invalid #10963

samsiegart commented Feb 7, 2025

0xpatrickdev left a comment

0xpatrickdev Feb 7, 2025

samsiegart Feb 11, 2025

0xpatrickdev Feb 11, 2025

samsiegart Feb 12, 2025

0xpatrickdev Feb 7, 2025

turadg Feb 7, 2025

turadg Feb 7, 2025

samsiegart Feb 11, 2025

turadg Feb 11, 2025

cloudflare-workers-and-pages bot commented Feb 8, 2025 •

edited

Loading

turadg Feb 11, 2025

turadg Feb 11, 2025

samsiegart Feb 12, 2025

turadg Feb 11, 2025

samsiegart Feb 12, 2025

turadg Feb 12, 2025

samsiegart left a comment

samsiegart Feb 12, 2025

samsiegart Feb 12, 2025

fix(fast-usdc): FORWARD_FAILED when EUD invalid #10963

fix(fast-usdc): FORWARD_FAILED when EUD invalid #10963

Conversation

samsiegart commented Feb 7, 2025

Description

Testing Considerations

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

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Feb 8, 2025 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Feb 8, 2025 •

edited

Loading