[ON HOLD] Save executed_surplus_fee to executed_fee_amount

[sunce86]

Description

[ON HOLD] Waiting for confirmation from the frontend team that they are showing executed_fee_amount for market orders and executed_surplus_fee for limit orders on UI.

In order to remove the executed_surplus_fee as per #2103, we first need to:

1. Merge this PR to staging and prod to make sure the executed_fee_amount is properly populated going forward.
2. Execute sql query on db to populate historic executed_fee_amount entries from executed_surplus_fee.

UPDATE order_execution
SET surplus_fee = solver_fee
WHERE surplus_fee IS NULL OR surplus_fee = 0;

3. Switch UI to read from executed_fee_amount
4. Merge Move executed_surplus_fee #2103

[sunce86]

order.sum_fee is 0 for limit orders

[fleupold]

This looks like a 1:1 copy of the full_order_into_model_order method. Can we call the shared one from the orderbook crate to remove the duplication?

[MartinquaXD]

This changes the meaning of the field. Are we fine with that?
Usually unless otherwise specified fee always relates to a fee that was signed by the user with their order, right?

[sunce86]

That was my intention. To have a single executed_fee field that will represent the fee that was paid for gas costs.
I don't see a reason to have two different fields for this.

[MartinquaXD]

I had a quick check and it doesn't look like the existing backend code base would break if we change that.
Also it doesn't appear to be stored in a DB anywhere (which would have made things weirder) so it really only seems to affect the frontend.
And for the frontend it's arguably more honest to show that we took some fees for executing the limit order.

[sunce86]

Frontend is currently showing the fee for limit orders, it's just reading it from the field I want to drop (the executed_surplus_fee).

[fleupold]

They are also showing the fee for all order types in a single value in the explorer. Are you sure their logic to derive this value allows breaking the existing sematics?
E.g. they could be adding executed_surplus_fee and executed_fee_amount (since one is 0 if the other one is set). This would now cause reporting 2x the fee for limit orders.

While not as drastic as dropping a field entirely, this is still a breaking change of the existing data model, so I think we should use caution.

[sunce86]

I asked them how they read that value and will wait for their confirmation before merging.
Although painful change, I still think this is a perfect timing to clean up the fees in our system before adding more (protocol fees etc).

[sunce86]

This looks like a 1:1 copy of the full_order_into_model_order method. Can we call the shared one from the orderbook crate to remove the duplication?

They do seem similar, one different thing is how the order status is calculated. Since I am not 100% sure we should merge these two, will do it in a separate PR so it could be reverted if needed.
#2110

[fleupold]

The PR description is missing context on what this PR is trying to achieve (it outlines the whole plan for the migration but no details for step 1) making it harder to review.

From my understanding we change the semantics of the executed_fee_amount field on the OrderMetadata struct which is used by the frontend.

Before, this value would be 0 for limit orders, whereas now, it will contain the executed_surplus_fee for limit orders (which is also available on order.class field for limit orders). Are we relying metadata.executed_fee_amount and executed_surplus_fee anywhere in the backend logic?

I think we should also check with the frontend team that this is not causing any issues in the explorer when displaying fees paid (we might always add up the two values).

[fleupold]

This looks like a 1:1 copy of the full_order_into_model_order method. Can we call the shared one from the orderbook crate to remove the duplication?

[sunce86]

Are we relying metadata.executed_fee_amount and executed_surplus_fee anywhere in the backend logic?

No, that was the main reason why I want to drop it, because it's used only for API (this is stated in the linked PR, so I didn't want to duplicate the explanations in this PR also).

[fleupold]

this is stated in the linked PR, so I didn't want to duplicate the explanations in this PR also)

I see. Personally, I expect @anxolin to look at this PR when he is back from vacation, so in order to make it easy for him to understand what this is about it may be nice to duplicate the context information but we can also require him to review #2103 as well to get a full picture.

[sunce86]

Closing as executed_fee_amount and executed_surplus_fee are not mutually exclusive and also we don't want to change the semantic expectation of the API.

More than happy to revisit this once we completely remove signed fee orders.