More descriptive error upon transfer simulation failure when placing order #1458

fleupold · 2021-12-05T12:15:08Z

We have gotten mutliple complaints from people trying to integrate with CowSwap that our order creation errors are the same (InsufficientFunds) in these two cases

User doesn't have enough balance of the token
User hasn't given sufficient allowance to the vault relayer

This PR makes these order creation errors distinct. In particular we use the following errors in the following scenarios:

InsufficientFunds - user doesn't have enough Balance
InsufficientAllowance - Vault Relayer doesn't have enough allowance on behalf of the user
SellAmountOverflows - sell amount (U256) + fee amount (U256) overflow U256 (very pathological case)
TransferSimulationFailure - transfer failed for other reasons (e.g. the token being paused at the moment)

In order to not create more roundtrips in the happy path (e.g. for frontends that make sure users cannot place orders without having given prior approval) we keep the transfer simulation (which covers all 3 cases in one call) and only check the exact failure reason in case it fails.

Test Plan

Added an integration test for the balance fetcher functions. It would be nice to use the order placement script with a custom endpoint (so I can easily create test orders locally)

…an order

codecov-commenter · 2021-12-05T12:24:33Z

Codecov Report

Merging #1458 (7181c72) into main (3eef426) will decrease coverage by 0.36%.
The diff coverage is 52.35%.

@@            Coverage Diff             @@
##             main    #1458      +/-   ##
==========================================
- Coverage   65.76%   65.40%   -0.37%     
==========================================
  Files         155      159       +4     
  Lines       30396    30049     -347     
==========================================
- Hits        19991    19654     -337     
+ Misses      10405    10395      -10

nlordell · 2021-12-05T13:09:00Z

crates/orderbook/src/api/order_validation.rs

+    // If fee and sell amount overflow u256
+    SellAmountOverflow,


I would be OK with a generic 500-error in these cases (unless this actually is happening to our users).

nlordell · 2021-12-05T13:15:45Z

crates/orderbook/src/api/order_validation.rs

+            return Ok((order, unsubsidized_fee));
+        }
+
+        if !self


Do you think it maybe makes sense to change BalanceFetching to have something like:

trait BalanceFetching { fn simulate_transfer(...) -> Result<(), TransferError>; } enum TransferError { InsufficientBalance, InsufficientAllowance, Other(...), }

The advantage of doing it this way is that we keep the details of testing transfers and the possible places it could go wrong in the BalanceFetching component.

Some other advantages:

It allows us to use a JSON RPC batch call for the balance and allowance query

We won't accidentally forget to update has_sufficient_balance to also pass in the SellTokenSource so that it works with Vault internal balances once we support it.

nlordell · 2021-12-05T13:17:59Z

crates/orderbook/src/api/order_validation.rs

+            .balance_fetcher
+            .has_sufficient_balance(order_creation.sell_token, owner, min_balance)
+            .await
+            .unwrap_or(false)
        {
            return Err(ValidationError::InsufficientFunds);


uber-naming-nit: This should probably be renamed to InsufficientBalance now.

@anxolin would renaming this error code be a breaking change for the FE?

Thanks @nlordell for notifying, indeed is relevant.

gnosis/cowswap#2010

nlordell · 2021-12-05T13:19:29Z

crates/orderbook/src/account_balances.rs

@@ -215,6 +227,32 @@ impl BalanceFetching for Web3BalanceFetcher {
        };
        Ok(success)
    }
+
+    async fn has_sufficient_balance(&self, token: H160, from: H160, amount: U256) -> Result<bool> {


In theory, this also needs to pass in the SellTokenSource because we would need to query the Vault::internal_balance in the case of SellTokenSource::Internal.

nlordell · 2021-12-05T13:22:25Z

crates/orderbook/src/account_balances.rs

+                instance.allowance(from, self.vault_relayer).call().await? >= amount
+            }
+            SellTokenSource::External => {
+                self.can_manage_user_balance_call(token, from, amount).await


One slight worry is that this has an implicit requirement that balance_fetcher.has_sufficient_balance() was previously checked, otherwise has_sufficient_allowance would generate false-positives when the user has a sufficient ERC20 allowance on the Vault and set relayer approval but has not more ERC20 token balance.

nlordell

In general code looks good modulo false positives in has_sufficient_allowance when the user has approval, but not enough ERC20 token balance. I think it should be possible to:

batch the ERC20::balance_of, ERC20::allowance and Vault::get_relayer_approval calls to determine the reason we can't transfer
hide the "complexities" of this check in the account_balances module and not have to "leak" it in the order creation logic.

bh2smith

Glad to finally have some refined error messages.

bh2smith · 2021-12-05T14:17:26Z

crates/orderbook/src/api/order_validation.rs

-            // TODO - None happens when checked_add overflows - not insufficient funds...
-            //  This error should be changed to SellAmountOverflow.
-            None => return Err(ValidationError::InsufficientFunds),
+            None => return Err(ValidationError::SellAmountOverflow),


Thanks for todo-ing this!

sunce86

Lgtm, although I would agree with @nlordell comment about converting return value of can_transfer from Result<bool> to Result<(), TransferError>, would make the code a bit clearer.

fleupold · 2021-12-07T10:23:16Z

I would agree with @nlordell comment about converting return value of can_transfer from Result to Result<(), TransferError>, would make the code a bit clearer.

I removed has_sufficient_balance and has_sufficient_allowance in favour of a more expressive can_transfer return type (should have already been there for the last two reviews).

Would you be able to take another look if this works now?

nlordell · 2021-12-07T10:32:54Z

crates/orderbook/src/account_balances.rs

+pub enum TransferSimulationResult {
+    Ok,
+    InsufficientAllowance,
+    InsufficientBalance,
+    TransferFailed,
+}


ubernit: Not a fan of these CustomResult types because they miss out on all the Result goodies (? support, all the standard library methods like map, unwrap_or_else, etc.) - I would much prefer:

Suggested change

pub enum TransferSimulationResult {

Ok,

InsufficientAllowance,

InsufficientBalance,

TransferFailed,

}

pub enum TransferSimulationError {

InsufficientAllowance,

InsufficientBalance,

TransferFailed,

Other(anyhow::Error),

}

And then return Result<(), TransferSimulationError>.

In terms of how to go from anyhow::Error to TransferSimulationError, you can either

impl From<anyhow::Error> for TransferSimulationError { fn from(err: anyhow::Error) -> Self { Self::Other(err) } }

Or, you can manually do it at the call site:

erc20_balance_query(&mut batch, token, from, self.vault_relayer) .await .map_err(TransferSimulationError::Other)?;

nlordell · 2021-12-07T10:36:02Z

crates/orderbook/src/api/order_validation.rs

@@ -748,7 +784,7 @@ mod tests {
                    .await
                    .unwrap_err()
            ),
-            "InsufficientFunds"
+            "SellAmountOverflow"


nit: completely unrelated to this PR (so feel free to not change that here), but this makes the test rely on how #[derive(Debug)] and seems a bit flaky to me... I think it would be much better to:

assert!(matches!( validator.validate_and_construct_order(...).await, Err(ValidationError::SellAmountOverflow), ));

nlordell

LGTM - just a small nit regarding the return type which feels a bit "un-Rust-y". Not blocking though.

nlordell

Awesome!

nlordell · 2021-12-07T13:04:04Z

crates/orderbook/src/api/order_validation.rs

+                })
+                .await,
+            Err(PartialValidationError::TransferEthToContract)
+        ));


# Summary Adds new errors for the backend. Now they have more granular description of the error. See gnosis/gp-v2-services#1458 (comment)

More descriptive error upon transfer simulation failure when placing …

d11d1f9

…an order

fleupold requested a review from a team December 5, 2021 12:15

nlordell reviewed Dec 5, 2021

View reviewed changes

fleupold added 2 commits December 5, 2021 20:10

moving logic into one funciton with enum as return type

24baa7b

InsufficientFunds -> InsufficientBalance

bcda0c2

bh2smith approved these changes Dec 6, 2021

View reviewed changes

sunce86 approved these changes Dec 6, 2021

View reviewed changes

Turning sellAmount overflow into 500 error

b1c8d71

nlordell reviewed Dec 7, 2021

View reviewed changes

nlordell approved these changes Dec 7, 2021

View reviewed changes

fleupold added 3 commits December 7, 2021 11:53

improve test

ab38a1e

improve another test

e5d138f

Make return type a standard result with custom simulation error

7181c72

nlordell approved these changes Dec 7, 2021

View reviewed changes

crates/orderbook/src/api/order_validation.rs

})

.await,

Err(PartialValidationError::TransferEthToContract)

));

Copy link

Contributor

nlordell Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

Merge branch 'main' into better_error_on_order_creation

928c60f

fleupold enabled auto-merge (squash) December 7, 2021 13:04

fleupold merged commit b6c9bb0 into main Dec 7, 2021

fleupold deleted the better_error_on_order_creation branch December 7, 2021 13:07

anxolin mentioned this pull request Dec 17, 2021

Add new errors: InsufficientAllowance, InsufficientBalance gnosis/cowswap#2010

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More descriptive error upon transfer simulation failure when placing order #1458

More descriptive error upon transfer simulation failure when placing order #1458

fleupold commented Dec 5, 2021

codecov-commenter commented Dec 5, 2021 •

edited

Loading

nlordell Dec 5, 2021

nlordell Dec 5, 2021 •

edited

Loading

nlordell Dec 5, 2021 •

edited

Loading

anxolin Dec 17, 2021

nlordell Dec 5, 2021

nlordell Dec 5, 2021

nlordell left a comment

bh2smith left a comment

bh2smith Dec 5, 2021

sunce86 left a comment

fleupold commented Dec 7, 2021

nlordell Dec 7, 2021

nlordell Dec 7, 2021

nlordell left a comment

nlordell left a comment

nlordell Dec 7, 2021

More descriptive error upon transfer simulation failure when placing order #1458

More descriptive error upon transfer simulation failure when placing order #1458

Conversation

fleupold commented Dec 5, 2021

Test Plan

codecov-commenter commented Dec 5, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

nlordell Dec 5, 2021 • edited Loading

Choose a reason for hiding this comment

nlordell Dec 5, 2021 • edited Loading

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

nlordell left a comment

Choose a reason for hiding this comment

bh2smith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunce86 left a comment

Choose a reason for hiding this comment

fleupold commented Dec 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlordell left a comment

Choose a reason for hiding this comment

nlordell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 5, 2021 •

edited

Loading

nlordell Dec 5, 2021 •

edited

Loading

nlordell Dec 5, 2021 •

edited

Loading