fix: min fulfillment amount for partial fulfillment #1570
Conversation
wischli
added
I2-bug
| fn to_asset_balance( | ||
| balance: Balance, | ||
| currency_in: CurrencyId, | ||
| ) -> Result<Balance, DispatchError> { | ||
| match currency_in { | ||
| CurrencyId::Native => Ok(balance), | ||
| CurrencyId::ForeignAsset(_) => { | ||
| let to_decimals = AssetRegistry::metadata(¤cy_in) | ||
| .ok_or(DispatchError::CannotLookup)? | ||
| .decimals; | ||
| convert_balance_decimals( | ||
| cfg_primitives::currency_decimals::NATIVE, | ||
| to_decimals, | ||
| balance, | ||
| ) | ||
| .map_err(DispatchError::from) | ||
| } | ||
| _ => Err(DispatchError::Token(sp_runtime::TokenError::Unsupported)), | ||
| } | ||
| } |
There was a problem hiding this comment.
Question 1: as I read this method, it seems like I want to transform a currency_in into an asset balance, but the implementation seems to do the opposite, transforming an asset balance to a native one. (Sure, I'm missing something). Is currency_in actually currency_out?
Question 2: Does it make sense to call this method with a native balance? Or should instead be an error?
There was a problem hiding this comment.
-
I followed the naming convention from the orderbook and foreign investments such that
currency_inrefers to the incoming asset for which we want the balance translation. I agree it is not optimal, probablycurrency_tomuch better. We could also just remove the direction and call itcurrency_id. WDYTH? -
Since calling it with native balance does not introduce any overhead, I think it should not error out because that makes it more flexible. However, I do not have a strong opinion here.
There was a problem hiding this comment.
-
Probably it's me that I feel dyslexic as with right or left 😆. I think I asked about this like 10 times haha. Given that the convention in other places is the same, we can leave it as it is.
currency_idalso fits 👍🏻 -
Ok, if semantically makes sense that I could call
to_asset_balance()using a native balance expecting no change on it, then I'm fine with the current implementation.
| // Order was fulfilled at least for invest swap amount | ||
| if status.amount > active_invest_swap_amount { | ||
| let invest_swap = SwapOf::<T> { | ||
| amount: active_invest_swap_amount, | ||
| ..status | ||
| }; | ||
| let redeem_swap = SwapOf::<T> { | ||
| amount: status.amount.ensure_sub(active_invest_swap_amount)?, | ||
| ..status | ||
| }; | ||
|
|
||
| // NOTE: Fulfillment of invest swap before redeem one for no particular reason. | ||
| // If we wanted to fulfill the min swap amount, we would have to add support for | ||
| // oppression of for swap updates to `fulfill_redeem_swap_order` as well in case | ||
| // redeem_swap.amount < status.amount < invest_swap.amount | ||
| Self::fulfill_invest_swap_order(&info.owner, info.id, invest_swap, false)?; | ||
| Self::fulfill_redeem_swap_order(&info.owner, info.id, redeem_swap) | ||
| } | ||
| // Order was fulfilled below invest swap amount | ||
| else { | ||
| Self::fulfill_invest_swap_order(&info.owner, info.id, status, true) | ||
| } |
There was a problem hiding this comment.
Do I read this correctly, that we are favouring Investment-swaps over Redepemtion-swaps? I.e. we take care of fulfilling investments first.
There was a problem hiding this comment.
Yes. Last night I started refactoring to prioritize lower orders instead, no matter if investment or redemption based. However, this requires adding support for oppressing updating swaps as part of fulfill_redeem_swap_order (i.e. apply_redeem_state_transition) similar to fulfill_invest_swap_order. This is quite tedious and creates more edge cases and should be tested thoroughly. Given that we would like to cut the release ASAP and me feeling confident with the current MVP, I propose to handle this in a subsequent PR.
There was a problem hiding this comment.
On a note: This favor of investment-swaps only occurs if both the invest state as well as redeem state are swapping into foreign such that the orderbook swap is "sum" of both.
There was a problem hiding this comment.
Thanks for the context.
Description
This PR mainly fixes issues for partial orderbook fulfillments of foreign investments as a successor of #1559. It also enables partial investments for orders created via extrinsics:
pallet-foreign-investmentspallet_order_book::create_orderandpallet_order_book::user_update_orderwhich had themin_fulfillment_amountset to thebuy_amountmaking partial fulfillments impossiblemin_fulfillment_amountfromTokenSwapstraitNativeBalanceDecimalConverterwhich provides means of converting between native and asset balance (i.e. re-denomination)MinFulfillmentAmountNativetopallet_order_book::Configwhich provides a global threshold which gets converted into the incoming asset precision via the also newpallet_order_book::Config::DecimalConvertertypeIntegration tests
Executed*dispatchesChecklist: