introduce new PriceEstimationError

[bh2smith]

While working on the new Fee & Quote endpoint, I noticed that our markets endpoint was returning an internal server error on something that would have been "BAD REQUEST"

Particularly,

https://protocol-mainnet.gnosis.io/api/v1/markets/0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48-0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2/buy/0

and

https://protocol-mainnet.gnosis.io/api/v1/markets/0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48-0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2/sell/0

Which should fail with a ZeroAmount error.

Test Plan

No logic changes, existing CI.

[vkgnosis]

I'm not a fan of having this logic be in shared and on the price estimation trait. Price estimation is specialized component that shouldn't care about about requests are handled, it shouldn't know about warp. If we want to have more code reuse (we don't have to) I'd like that to be a in the api module only.

[bh2smith]

I'm not a fan of having this logic be in shared and on the price estimation trait.

Totally fair point. The issue I ran into when trying to implement to_warp_reply for this in the correct place was that I wasn't allowed to implement logic outside the module where the struct was declared. Do you have some suggestion on how this can be more appropriately done?

[vkgnosis]

You can implement a function price_estimation_error_to_warp_reply(error: PriceEstimationError -> ... . You just can't implement it directly on PriceEstimationError. Or you could create a new trait with this function and then impl the trait for the type. The rule is that either the type or the trait have to be in the local crate. For a trait is probably overkill so I'd do the free function.

[nlordell]

Another option is to implement From:

impl From<PriceEstimationError> for Error<'static> {
    // ...
}

I'm also a fan of just a floating function in this case (I think trait might be a tad overkill here).

[bh2smith]

Cool folks thank for the comments. I will go with the free floating function (for now). One the bigger PR has the trait, I can adjust it then.

[bh2smith]

Fixed this - now includes only relevant changes to the Missing Price Estimation Error.

[codecov-commenter]

Codecov Report

Merging #1188 (4b9adb4) into main (e0e077d) will increase coverage by 0.10%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##             main    #1188      +/-   ##
==========================================
+ Coverage   67.15%   67.26%   +0.10%     
==========================================
  Files         135      135              
  Lines       27210    27189      -21     
==========================================
+ Hits        18273    18288      +15     
+ Misses       8937     8901      -36     

[nlordell]

Maybe Error type should be:

enum Error {
    SellAmountDoesNotCoverFee,
    PriceEstimate(PriceEstimationError),
}

This would avoid duplicated error messages as well as error variants.

[nlordell]

Looks good. Tiny nit.