unconditional event and post in withdraw by thedavidmeister · Pull Request #1959 · rainlanguage/raindex

thedavidmeister · 2025-06-30T18:17:37Z

Motivation

Solution

Checks

By submitting this for review, I'm confirming I've done the following:

made this PR as small as possible
unit-tested any new functionality
linked any relevant issues or PRs
included screenshots (if this involves a front-end change)

Summary by CodeRabbit

Refactor
- Improved the clarity and structure of the withdrawal process, ensuring the withdrawal event is always emitted after token transfers, even when no tokens are withdrawn.
Bug Fixes
- Updated tests to correctly expect withdrawal event emissions in all scenarios, including zero or empty vault withdrawals.
- Enhanced test assertions with clearer error messages and conditional revert handling for more accurate validation.

coderabbitai · 2025-06-30T18:18:25Z

## Walkthrough

The `withdraw2` function in `OrderBook.sol` was updated to emit the `Withdraw` event after the token transfer, ensuring the event is always emitted regardless of the withdrawal amount. Formatting and indentation of the post-processing logic were also adjusted for clarity, without altering functionality. Test contracts were enhanced to check for event emission even on zero or empty withdrawals and to handle expected revert errors with improved assertion messages.

## Changes

| File(s)                                     | Change Summary                                                                                                                                    |
|---------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------|
| src/concrete/ob/OrderBook.sol                | Moved `Withdraw` event emission to after token transfer and outside conditional; adjusted formatting and indentation of post-processing logic.  |
| test/concrete/ob/OrderBook.withdraw.entask.t.sol | Enhanced `checkWithdraw` with revert error handling and descriptive assertions; updated calls to handle conditional revert expectations and event checks. |
| test/concrete/ob/OrderBook.withdraw.t.sol    | Updated `testWithdrawEmptyVault` to expect `Withdraw` event emission even on empty vault withdrawal; adjusted log assertions accordingly.         |

## Sequence Diagram(s)

```mermaid
sequenceDiagram
    participant User
    participant OrderBook
    participant Token

    User->>OrderBook: Call withdraw2(amount, ...)
    OrderBook->>OrderBook: Update vault balance
    alt amount > 0
        OrderBook->>Token: Transfer tokens to User
    end
    OrderBook->>User: Emit Withdraw event (always)
    OrderBook->>OrderBook: Process post array (if any)


<!-- walkthrough_end -->


---

<details>
<summary>📜 Recent review details</summary>

**Configuration used: CodeRabbit UI**
**Review profile: ASSERTIVE**
**Plan: Pro**


<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between bcf8a3d15e59ae48a7d5085ed26c899690c0ace0 and 8ea00c3775ddd5e2412befc78682828534cdbfcc.

</details>

<details>
<summary>📒 Files selected for processing (2)</summary>

* `test/concrete/ob/OrderBook.withdraw.entask.t.sol` (12 hunks)
* `test/concrete/ob/OrderBook.withdraw.t.sol` (1 hunks)

</details>

<details>
<summary>🧰 Additional context used</summary>

<details>
<summary>🧠 Learnings (3)</summary>

<details>
<summary>📓 Common learnings</summary>

Learnt from: rouzwelt
PR: rainlanguage/rain.orderbook#0
File: :0-0
Timestamp: 2025-05-21T23:34:42.328Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, the PR size report must use the format:
TOTAL:number
ADDITIONS:number
DELETIONS:number
with no extra text, formatting blocks, or explanations, and must still exclude lock files and reassess after each new commit.

Learnt from: rouzwelt
PR: rainlanguage/rain.orderbook#0
File: :0-0
Timestamp: 2025-05-21T22:46:08.530Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, the PR size report should be the only content in the comment - no text before it, no text after it, no formatting blocks, just the raw report in the exact format: "TOTAL=number\nADDITIONS=number\nDELETIONS=number". The report must exclude lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock).

Learnt from: rouzwelt
PR: rainlanguage/rain.orderbook#0
File: :0-0
Timestamp: 2025-05-21T21:26:31.566Z
Learning: For PR #1884 and future PRs in rainlanguage/rain.orderbook repository, provide PR size reports in a separate comment with ONLY the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number", without any additional text or explanation. Reports should exclude irrelevant files such as lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock).

Learnt from: rouzwelt
PR: rainlanguage/rain.orderbook#0
File: :0-0
Timestamp: 2025-05-21T22:14:22.646Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, I must consistently report the PR size after EVERY new commit in the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number" without any additional text or formatting blocks. The report must exclude lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock) and can be included either as part of the review or as a separate comment.

Learnt from: rouzwelt
PR: rainlanguage/rain.orderbook#0
File: :0-0
Timestamp: 2025-05-21T22:14:22.646Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, I must consistently report the PR size after EVERY new commit in the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number" without any formatting blocks. The report must exclude lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock) and can be included either as part of the review or as a separate comment.

Learnt from: rouzwelt
PR: rainlanguage/rain.orderbook#0
File: :0-0
Timestamp: 2025-05-21T22:47:21.927Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, the PR size report must always be placed at the very beginning of any comment, before any learning used section or other content.

Learnt from: rouzwelt
PR: rainlanguage/rain.orderbook#0
File: :0-0
Timestamp: 2025-05-21T22:35:26.448Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, the PR size report must always be placed at the very last part of any comment, after any learning used section or other content.

Learnt from: rouzwelt
PR: rainlanguage/rain.orderbook#0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: TOTAL=number ADDITIONS=number DELETIONS=number, excluding lock files like package-lock.json and cargo.lock.

Learnt from: hardingjam
PR: #1504
File: tauri-app/src/routes/orders/[network]-[orderHash]/+page.svelte:33-37
Timestamp: 2025-04-08T16:35:15.127Z
Learning: In the Rain OrderBook project, the onDeposit and onWithdraw functions in page components are kept simple (just calling modal handlers and revalidating queries) because error handling for deposit and withdraw actions (including user cancellations and failed transactions) is handled within the modal components themselves.

Learnt from: thedavidmeister
PR: #1926
File: src/concrete/ob/OrderBook.sol:97-99
Timestamp: 2025-06-16T10:46:39.129Z
Learning: In OrderBook.sol, thedavidmeister prefers error definitions that are self-descriptive when the error condition is specific enough. For ClearZeroAmount, since it only fires when both output amounts are zero, the error name itself provides sufficient context without needing to include the zero values as parameters.

Learnt from: hardingjam
PR: #1504
File: tauri-app/src/routes/orders/[network]-[orderHash]/+page.svelte:39-43
Timestamp: 2025-04-08T16:35:41.663Z
Learning: In the Rain OrderBook application, the user has confirmed that error handling for withdrawal cancellations is handled in the modal tests, not in the page-level code, consistent with the architectural pattern where transaction-related error handling is centralized in modal components.

Learnt from: thedavidmeister
PR: #1926
File: test/concrete/ob/OrderBook.clear.zeroAmount.t.sol:24-32
Timestamp: 2025-06-16T10:49:47.770Z
Learning: LibTestAddOrder.conformConfig() in test/util/lib/LibTestAddOrder.sol automatically constrains OrderConfigV3 to prevent common test failures by ensuring validInputs[0].token != validOutputs[0].token, setting them to address(0) and address(1) respectively if they're equal. This prevents TokenSelfTrade errors in fuzz tests.

Learnt from: hardingjam
PR: #1870
File: packages/webapp/src/tests/WithdrawModal.test.ts:74-97
Timestamp: 2025-05-20T12:08:13.760Z
Learning: In the WithdrawModal test suite, testing that transactions are correctly passed to the transactionStore is considered sufficient coverage, without needing explicit tests for callback execution.

Learnt from: findolor
PR: #1872
File: packages/webapp/src/tests/handleVaultDeposit.test.ts:20-53
Timestamp: 2025-06-07T05:19:46.330Z
Learning: In the rain.orderbook codebase, simple wrapper/adapter functions that just delegate to other functions (like handleVaultDeposit) don't need extensive edge case testing for missing parameters or error handling - the current test coverage focusing on core functionality is sufficient.

Learnt from: findolor
PR: #1907
File: packages/orderbook/test/common/test.test.ts:75-77
Timestamp: 2025-06-04T10:21:01.388Z
Learning: The DotrainOrder.create API in packages/orderbook/test/common/test.test.ts is internal and not used directly in consumer applications, so API changes here don't require external breaking change documentation.

Learnt from: findolor
PR: #1872
File: packages/webapp/src/lib/services/handleVaultWithdraw.ts:74-76
Timestamp: 2025-06-07T05:18:31.404Z
Learning: In vault withdrawal error handling, prefer user-friendly generic error messages over exposing technical error details from getVaultWithdrawCalldata, as the underlying errors are not end user friendly.

Learnt from: hardingjam
PR: #1896
File: tauri-app/src/lib/services/executeWalletConnectOrder.ts:41-45
Timestamp: 2025-05-27T12:01:51.647Z
Learning: In the tauri-app codebase, the formatEthersTransactionErrorFn function already handles error formatting appropriately for toast messages in executeWalletConnectOrder, so additional granular error handling based on error types is not needed.

Learnt from: findolor
PR: #1744
File: packages/webapp/src/lib/components/DepositOrWithdrawModal.svelte:73-76
Timestamp: 2025-05-09T05:30:02.520Z
Learning: In Rain.orderbook, VaultCalldataResult is a wrapper type that contains both error and value properties. When using this in the DepositOrWithdrawModal component, only the inner value field is passed to the handleTransaction function, not the complete wrapper.


</details>
<details>
<summary>test/concrete/ob/OrderBook.withdraw.t.sol (12)</summary>

Learnt from: findolor
PR: #1907
File: packages/orderbook/test/common/test.test.ts:75-77
Timestamp: 2025-06-04T10:21:01.388Z
Learning: The DotrainOrder.create API in packages/orderbook/test/common/test.test.ts is internal and not used directly in consumer applications, so API changes here don't require external breaking change documentation.

Learnt from: findolor
PR: #1872
File: packages/webapp/src/tests/handleVaultDeposit.test.ts:20-53
Timestamp: 2025-06-07T05:19:46.330Z
Learning: In the rain.orderbook codebase, simple wrapper/adapter functions that just delegate to other functions (like handleVaultDeposit) don't need extensive edge case testing for missing parameters or error handling - the current test coverage focusing on core functionality is sufficient.

Learnt from: hardingjam
PR: #1870
File: packages/webapp/src/tests/WithdrawModal.test.ts:74-97
Timestamp: 2025-05-20T12:08:13.760Z
Learning: In the WithdrawModal test suite, testing that transactions are correctly passed to the transactionStore is considered sufficient coverage, without needing explicit tests for callback execution.

Learnt from: thedavidmeister
PR: #1926
File: test/concrete/ob/OrderBook.clear.zeroAmount.t.sol:24-32
Timestamp: 2025-06-16T10:49:47.770Z
Learning: LibTestAddOrder.conformConfig() in test/util/lib/LibTestAddOrder.sol automatically constrains OrderConfigV3 to prevent common test failures by ensuring validInputs[0].token != validOutputs[0].token, setting them to address(0) and address(1) respectively if they're equal. This prevents TokenSelfTrade errors in fuzz tests.

Learnt from: findolor
PR: #1744
File: packages/webapp/src/lib/components/DepositOrWithdrawModal.svelte:73-76
Timestamp: 2025-05-09T05:30:02.520Z
Learning: In Rain.orderbook, VaultCalldataResult is a wrapper type that contains both error and value properties. When using this in the DepositOrWithdrawModal component, only the inner value field is passed to the handleTransaction function, not the complete wrapper.

Learnt from: hardingjam
PR: #1559
File: packages/ui-components/src/tests/OrderOrVaultHash.test.ts:94-94
Timestamp: 2025-04-04T11:25:21.518Z
Learning: In the rain.orderbook project, minimal test fixtures are preferred over complete mocks that implement the entire interface. Type casting (e.g., as unknown as SgVault) is an acceptable approach to maintain both minimal fixtures and TypeScript type compatibility.

Learnt from: findolor
PR: #1872
File: packages/webapp/src/lib/services/handleVaultWithdraw.ts:74-76
Timestamp: 2025-06-07T05:18:31.404Z
Learning: In vault withdrawal error handling, prefer user-friendly generic error messages over exposing technical error details from getVaultWithdrawCalldata, as the underlying errors are not end user friendly.

Learnt from: hardingjam
PR: #1504
File: tauri-app/src/routes/orders/[network]-[orderHash]/+page.svelte:39-43
Timestamp: 2025-04-08T16:35:41.663Z
Learning: In the Rain OrderBook application, the user has confirmed that error handling for withdrawal cancellations is handled in the modal tests, not in the page-level code, consistent with the architectural pattern where transaction-related error handling is centralized in modal components.

Learnt from: findolor
PR: #1891
File: packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/page.test.ts:66-80
Timestamp: 2025-06-08T18:43:51.842Z
Learning: In the rain.orderbook webapp test files, when mocking objects like the transaction manager, it's acceptable to use simple empty objects with @ts-expect-error comments rather than providing complete mock implementations with all properties and methods.

Learnt from: hardingjam
PR: #1504
File: tauri-app/src/routes/orders/[network]-[orderHash]/+page.svelte:33-37
Timestamp: 2025-04-08T16:35:15.127Z
Learning: In the Rain OrderBook project, the onDeposit and onWithdraw functions in page components are kept simple (just calling modal handlers and revalidating queries) because error handling for deposit and withdraw actions (including user cancellations and failed transactions) is handled within the modal components themselves.

Learnt from: hardingjam
PR: #1860
File: packages/ui-components/src/lib/stores/transactionStore.ts:130-136
Timestamp: 2025-05-19T18:24:17.608Z
Learning: In the rain.orderbook project, the getTransaction function (and related entity fetch functions) use wasmEncodedResult to validate the returned data, ensuring that only valid data is returned. This makes a simple truthiness check (!!data) sufficient for the isSuccess predicate in awaitSubgraphIndexing.

Learnt from: thedavidmeister
PR: #1926
File: src/concrete/ob/OrderBook.sol:97-99
Timestamp: 2025-06-16T10:46:39.129Z
Learning: In OrderBook.sol, thedavidmeister prefers error definitions that are self-descriptive when the error condition is specific enough. For ClearZeroAmount, since it only fires when both output amounts are zero, the error name itself provides sufficient context without needing to include the zero values as parameters.


</details>
<details>
<summary>test/concrete/ob/OrderBook.withdraw.entask.t.sol (16)</summary>