test(fast-usdc): multichain test for forward #10963
Conversation
| // 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'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| let dest; | ||
| try { | ||
| dest = chainHub.makeChainAddress(EUD); | ||
| } catch (e) { | ||
| log('⚠️ forward transfer failed!', e, txHash); | ||
| return statusManager.forwarded(txHash, false); | ||
| } |
There was a problem hiding this comment.
The mutable dest is a smell.
Not a blocker for this testing PR, but I think we need a more general error handling solution.
Also not a blocker but this change should not be part of a test commit. It's a runtime change, specifically a fix.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
If that's the case let's wait until #10811
Deploying agoric-sdk with
|
| Latest commit: |
a1f13a5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6dab6bb2.agoric-sdk.pages.dev |
| Branch Preview URL: | https://test-forward-fail.agoric-sdk.pages.dev |
samsiegart
force-pushed
the
test-forward-fail
branch
from
6024101 to
616fab6
Compare
samsiegart
force-pushed
the
test-forward-fail
branch
from
616fab6 to
a1f13a5
Compare
| let dest; | ||
| try { | ||
| dest = chainHub.makeChainAddress(EUD); | ||
| } catch (e) { | ||
| log('⚠️ forward transfer failed!', e, txHash); | ||
| return statusManager.forwarded(txHash, false); | ||
| } |
There was a problem hiding this comment.
statusManager.forwarded is void so we don't need to return it. Let's keep the const. One way:
| 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.
There was a problem hiding this comment.
Separately, this change is appearing in a test commit but it changes the runtime behavior. It's a fix or feat.
| const maxRetries = 5; | ||
| const retryDelayMS = 500; |
There was a problem hiding this comment.
don't we have a general "retry promise up to N times with M delay" utility? cc @dckc
closes: #10623
Description
Adds multichain-testing scenarios for:
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.