Surface transfer simulation revert reasons in order creation API#4321
Surface transfer simulation revert reasons in order creation API#4321squadgazzz merged 15 commits intomainfrom
Conversation
849c9b0 to
f618907
Compare
354c05b to
01076f4
Compare
There was a problem hiding this comment.
Code Review
This pull request enhances the balance simulation by capturing and propagating the revert reason when a transfer fails. Changes span the Solidity contract, generated bindings, and the backend validation logic. Feedback was provided regarding the error mapping in the order validator, where internal simulation errors are currently converted into client-side validation failures, potentially masking RPC or node issues and returning incorrect status codes.
| TransferSimulationError::Other(err) => { | ||
| tracing::warn!("TransferSimulation failed: {:?}", err); | ||
| Err(ValidationError::TransferSimulationFailed) | ||
| Err(ValidationError::TransferSimulationFailed(Vec::new())) |
There was a problem hiding this comment.
Mapping TransferSimulationError::Other to ValidationError::TransferSimulationFailed(Vec::new()) is potentially misleading. If the simulation failed due to an internal error (e.g., node connection issue, RPC failure), it should ideally be propagated as ValidationError::Other to trigger a 500 Internal Server Error instead of a 400 Bad Request. While this preserves existing behavior, the addition of the empty Vec highlights that we are losing the actual error context here.
| Err(ValidationError::TransferSimulationFailed(Vec::new())) | |
| Err(ValidationError::Other(err)) |
The return type now includes `bytes` (a dynamic type), so the tuple is dynamically encoded. `abi_decode` wraps data in a single-element tuple expecting an offset prefix, while `abi_decode_params` correctly decodes the raw ABI-encoded tuple from simulateDelegatecall.
Mainnet: 23112197/23326100/23688436 -> 24843565 Gnosis: 41502478 -> 45588623
The previous whale (0x762d) ran out of ETH by block 24843565. Use Binance (0x28c6) which has both DAI and ETH.
There was a problem hiding this comment.
Code Review
This pull request updates the Balances contract and simulation infrastructure to capture and return the revert reason when a transfer simulation fails. The changes involve Solidity contract updates, Rust binding regenerations, and API response modifications. Feedback was provided regarding the truncation of revert data in the API response; instead of providing only the first four bytes, the full revert data should be returned to enable clients to decode standard error strings and panic reasons.
Include the entire revert_data hex instead of just the 4-byte selector, so clients can decode standard Error(string) and Panic(uint256) reasons.
Summary
TransferSimulationFailed, the API now includes the actual revert reason (e.g.0x5b263df7=LtvValidationFailed()) instead of the generic "sell token cannot be transferred"Balances.solhelper contract to capture revert bytes from thetransferFromAccountstry/catch blockSimulation→TransferSimulationError::TransferFailed(Vec<u8>)→ValidationError::TransferSimulationFailed(Vec<u8>)→ API error description0x88b4B74082BffB2976C306CB3f7E9093AE48B94Fon all chains (except Lens which keeps the old deployment)Motivation
Aave aTokens (e.g. aBasUSDC) revert with
LtvValidationFailed()when a user has zero-LTV collateral blocking transfers. The current generic error gives no indication of why the transfer fails, making it very difficult to debug.API Change
Before:
{"errorType": "TransferSimulationFailed", "description": "sell token cannot be transferred"}After (when revert data available):
{"errorType": "TransferSimulationFailed", "description": "sell token cannot be transferred, token reverted with: 0x5b263df7"}The 4-byte selector can be decoded client-side (e.g.
cast 4byte 0x5b263df7→LtvValidationFailed()).Changes
contracts/solidity/Balances.sol—catch→catch (bytes memory reason), addedtransferRevertReasonreturn valuecontracts/artifacts/Balances.json— Rebuilt with solccrates/account-balances/src/lib.rs— Addedtransfer_revert_reasontoSimulation, changedTransferFailedto carryVec<u8>, switched toabi_decode_params(required for dynamic return types)crates/account-balances/src/simulation.rs— Pass revert bytes intoTransferFailedcrates/shared/src/order_validation.rs— ThreadVec<u8>throughTransferSimulationFailedcrates/orderbook/src/api/post_order.rs— Format revert selector in error descriptioncontracts/src/main.rs— Updated Balances addresses to new deployment0x88b4B74082BffB2976C306CB3f7E9093AE48B94FJustfile— Added formatting steps togenerate-contractsNew Balances Deployment
Address
0x88b4B74082BffB2976C306CB3f7E9093AE48B94Fon: Mainnet, Arbitrum, Base, Avalanche, BNB, Optimism, Polygon, Gnosis, Linea, Ink, Sepolia, Plasma. And also Optimism, since we deployed everything there already and in case we will add support for this chain sometime later.Lens keeps the old address (
0x3e8C6De9510e7ECad902D005DE3Ab52f35cF4f1b), since it will be dropped from the codebase in a follow-up PR.Test plan