Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comprehensive testing of failure scenarios #10510

Closed
LuqiPan opened this issue Nov 18, 2024 · 5 comments
Closed

Comprehensive testing of failure scenarios #10510

LuqiPan opened this issue Nov 18, 2024 · 5 comments
Assignees

Comments

@LuqiPan
Copy link
Contributor

LuqiPan commented Nov 18, 2024

What is the Problem Being Solved?

Description of the Design

RunUtils test covering happy path and error paths (from product)

Error paths from implementation and upstream service irregularities

Race conditions

Verify vstorage output

Security Considerations

Scaling Considerations

Test Plan

Upgrade Considerations

@0xpatrickdev
Copy link
Member

A lot of work was recently done around potential failure scenarios wrt to StatusManager. Namely, accounting for failed IBC transfers and unexpected timings around advance and settlement.

The bulk of the above was captured in #10530 which also included diagrams of the state machine:
https://github.com/Agoric/agoric-sdk/blob/23120a923c2bd3ca182291df2080214fbd5f20ca/packages/fast-usdc/README.md#status-manager

At the time of writing, there is a mistake in the Minted -> Forwarded step. Mints should wait for Observe before proceeding.

@0xpatrickdev
Copy link
Member

At the time of writing, there is a mistake in the Minted -> Forwarded step. Mints should wait for Observe before proceeding.

This might suggest we need an additional state - perhaps Minted - to capture "minted before observed". If it's not a requirement of the state machine internals, we might want it for vstroage reporting (under a new key since we won't have the evm txHash).

This surfaced when discussing #10508 with @samsiegart. Our current plan is only to report TxStatus statuses (below) to vstorage and log the "minted before observed" events to the swingset console.

export const TxStatus = /** @type {const} */ ({
/** tx was observed but not advanced */
Observed: 'OBSERVED',
/** IBC transfer is initiated */
Advancing: 'ADVANCING',
/** IBC transfer is complete */
Advanced: 'ADVANCED',
/** IBC transfer failed (timed out) */
AdvanceFailed: 'ADVANCE_FAILED',
/** settlement for matching advance received and funds disbursed */
Disbursed: 'DISBURSED',
/** fallback: do not collect fees */
Forwarded: 'FORWARDED',
/** failed to forward to EUD */
ForwardFailed: 'FORWARD_FAILED',
});
harden(TxStatus);

@dckc
Copy link
Member

dckc commented Nov 26, 2024

In discussion of #10572 / #10388, we agreed to postpone this one to here:

  • [todo] C12 - Contract MUST only pay back the Pool only if they started the advance before USDC is minted

mergify bot added a commit that referenced this issue Nov 27, 2024
refs: #10388
closes: #10577

doesn't close until
 - exercise all the code in the contract (analytic; best effort)

## Description / Testing Considerations

 - [x] test all the sequences in the product spec
    - [x] enumerate all the sequences in the product spec
 - [x] fix advancer/settler amount agreement bug

postponed:
 - 
 - #10510
    - [todo] C12 - Contract MUST only pay back the Pool only if they started the advance before USDC is minted
 - #10554

some `test.todo()`s are out of scope of contract flow tests. Some are more feasible as exo tests:
  - [todo] C18 - forward - MUST log and alert these incidents
  - [skip] LP borrow - TODO: move to exo test
  - [skip] LP repay - TODO: move to exo test

an some are after M1:
  - [todo] PERF: Target: settlement completes in a few minutes (after USDC is minted)
  - [todo] fee levels MUST be visible to external parties - i.e., written to public storage
  - [todo] C21 - Contract MUST log / timestamp each step in the transaction flow

### Security / Scaling / Upgrade Considerations

n/a

### Documentation Considerations

Internal design docs are assumed background knowledge.
@0xpatrickdev
Copy link
Member

0xpatrickdev commented Dec 4, 2024

Task List

I suggest we create separate issues for each and close this once were in agreement:

Priority Task Details
1 Wrap vow handlers that create promises • Some onFulfilled and onRejected handlers can throw; these failures will be silent (no unhandled promise rejection logs) until #10576
@dckc suggests wrapping anything that can throw in try/catch for more graceful capture
2 Boot tests for Tx status updates and Pool Metrics • Covers requirements around visibility for happy and failure paths from off-chain perspective
• All paths/states should be tested in bootstrap
3 Debug logging coverage • vstorage writes do not generate a console log
• Need observability through TX lifecycle in console logs
• Current coverage is decent but could improve in: eventFeed, statusManager
4 localTransfer, withdrawToSeat failure paths • Unexpected case but low effort to return payment to LP
• Task involves wiring up this return mechanism
5 FORWARD_FAILED state • Transition to Forwarded on success, not initation
• Add state for ForwardFailed

Google Doc where much of this was fleshed out. There are many other paths around LP share path, fee calcs, and unexpected sequences we already have sufficient coverage for.

Tickets:

  1. Boot Tests + Debug Logging (combined 2 +3)
  2. localTransfer, withdrawToSeat failure paths
  3. FORWARD_FAILED state

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants