New fee and amount quote API

[nlordell]

This PR proposes a new API enpoint for quoting fees and prices for orders in a way that more complete order data is specified. This allows the fee quoting mechanism to take app ID as well as balance sources (i.e. Vault balances for example) to be taken into account. In the future, things like the order owner can also be checked to see if they are staking some GP tokens in order to give them better fees as well.

Test Plan

This PR just introduces a new end point and the OpenAPI specification for it. The implementation will come in a future PR.

ALso, you can test that the new route returns a 500 error with message "not implemented":

$ curl -s \
  -X POST \
  -H 'Content-Type: application/json' \
  --data '{"from":"0x0101010101010101010101010101010101010101","sellToken":"0x0202020202020202020202020202020202020202","buyToken":"0x0303030303030303030303030303030303030303","receiver":"0x0404040404040404040404040404040404040404","kind":"buy","buyAmountAfterFee":"1337","validTo":12345678,"appData":"0x9090909090909090909090909090909090909090909090909090909090909090","partiallyFillable":false}' \
  'http://localhost:8080/api/v1/quote'
{"errorType":"InternalServerError","description":"not yet implemented"}

[fleupold]

I like that this will give us full control to implement arbitrary fee logic based on the order data (without requiring more updates in the frontends). We just need to make sure we consolidate/clean up our various fee endpoints.

[nlordell]

We just need to make sure we consolidate/clean up our various fee endpoints.

We can add a DEPRECATED tag to the old fee end points, and then refactor so that they ultimately route to the new endpoint under the hood.

[vkgnosis]

I like this api and also think we should remove the other fee related endpoints then. Would like to hear from frontend too what they think about this api.

[anxolin]

Not sure if you are aware but we already provide the AppId using a custom header:

I think this can be an "official" option too, to allow to provide custom headers.

Happy to make it part of the params, so it's even more official as you suggest in this PR

[nlordell]

I think this can be an "official" option too, to allow to provide custom headers.

@anxolin What we ultimately want though, is to have custom logic for determining fees based on the full order parameters and not just the app ID. So in this case, the header is not enough.

For example, we may want to check what the sellTokenBalance and buyTokenBalance are, so order that trade internal buffer balances tokenized on L2 would pay 0 fees (Martin's idea from the retreat). Another example would be to also have a fee discount based on the user address, so a user that stakes GP token gets a discount.

So the idea of the API is to pass as much of the order data as possible to the price/fee computation end point. This way we are more "future proof" and don't need to keep adding new header parameters as we consider different order parameters for determining the fee subsidy. I hope this makes sense 😄.

[bh2smith]

Love this. Happy to implement the test once this has landed!

[bh2smith]

If this is essentially considered set in stone, then I would start implementing this feature on top of this branch. Will wait, however, on the go ahead.

[anxolin]

This way we are more "future proof" and don't need to keep adding new header parameters as we consider different order parameters for determining the fee subsidy. I hope this makes sense

Sure it does 👌

[bh2smith]

I can't see the difference here between 400 and 500 errors - both are "Error quoting order".

Also, is the pattern for #/components/schemas/FeeAndQuoteError defined here, because it doesn't appear to be.

I think we may also be missing some possibilities here. For example;

• UnsupportedToken(H160),
• TransferEthToContract,
• SameBuyAndSellToken,
• UnsupportedBuyTokenDestination(BuyTokenDestination),
• UnsupportedSellTokenSource(SellTokenSource),
• ZeroAmount,

I can't really see the reason for the 429 here, unless we mean to change this to "RateLimited" too many quotes? but this could easily be worked around.

[bh2smith]

Might be cute to encapsulate all these different specifics into the single FeeAndQuoteError which i imagine is the intention here. Something like

pub enum PostQuoteResult {
    QuoteConstructed(OrderQuote),
    FeeAndQuoteError(FeeError),
    Forbidden,
}

pub enum FeeError {
    UnsupportedToken(H160),
    TransferEthToContract,
    SameBuyAndSellToken,
    UnsupportedBuyTokenDestination(BuyTokenDestination),
    UnsupportedSellTokenSource(SellTokenSource),
    ZeroAmount,
}

[nlordell]

#/components/schemas/FeeAndQuoteError already exists (ans is used by the feeAndQuote route). Since the errors are the same, I just reused the error name for now.

Additionally, we would probably reuse the error type from the existing feeAndQuote route.

[bh2smith]

That sounds reasonable, but what about all these other responses that seem out of place

        403:
          description: Forbidden, your account is deny-listed
        429:
          description: Too many order placements
        500:
          description: Error quoting an order

as I see it, the only things we will need here are

#[derive(Serialize)]
pub enum OrderQuoteResult {
    QuoteConstructed(OrderQuote),
    FeeAndQuoteError(FeeAndQuoteError),
}

along with some possible rate limiting...

[nlordell]

along with some possible rate limiting...

That is what the 429 is for AFIAU:

        429:
          description: Too many order placements # i.e you quoted too many orders and are being rate limited. We should `s/placements/quotes/` to make the error a bit clearer.

Additionally, 500 is used as a catch all "internal server error" - i.e. something unexpected went wrong.

Finally:

403:
          description: Forbidden, your account is deny-listed

Since the from is passed in, we can also check that the owner is forbidden by address (we have an "owner deny list" already).

[nlordell]

Fixed 429 description and added "unexpected" to 500 description.

[bh2smith]

There appears to be something missing in the description of 429 here in your comment...

We should `s/placements/quotes/` to make the error a bit clearer.

[nlordell]

It was changed to:

        429:
          description: Too many order quotes

[bh2smith]

I have opened a draft PR #1155 based on this branch which essentially fills in the unimplemented code routes with the existing fee calculation logic. I know there is an issue listing some of the requirements for the new fee logic, but can't seem to find it.

[nlordell]

based on this branch which essentially fills in the unimplemented code routes with the existing fee calculation logic

I will do some final fix ups and merge this then.

[anxolin]

@nlordell let us know when we this hits production, or when u want us to start using the new endpoint

Also, we should let Balancer know too, right?

[nlordell]

Also, we should let Balancer know too, right?

Its still a little off from being implemented (probably ~ 2 weeks away).