Ensure that trade_preimage will not succeed on input that will fail on buy/sell/setprice #937
Conversation
sergeyboyko0791
commented
May 7, 2021
sergeyboyko0791
commented
- GenerateTxError::OutputValueLessThanDust converts to TradePreimageError::InternalError the value is upper bound
- Fix invalid required field in NotSufficientBalance error generated by calc_max_maker_vol
…n setprice * Add balance checking on trade_preimage::method = setprice * Add an additional params checking using MakerOrderBuilder * Fix broken tests, add red test_trade_preimage_additional_validation test
…n sell/buy * Add balance checking on trade_preimage::method = sell/buy * Add an additional params checking using TakerOrderBuilder * Make the test_trade_preimage_additional_validation test green * Extend test_trade_preimage_not_sufficient_balance
… calc_max_maker_vol * Fix broken tests * Extend the test_trade_preimage_not_sufficient_balance test * Add the test_trade_preimage_deduct_fee_from_output_failed test
* GenerateTxError::OutputValueLessThanDust converts to TradePreimageError::InternalError the value is upper bound
mm2src/lp_swap/trade_preimage.rs
Outdated
| price: actual.to_decimal(), | ||
| threshold: threshold.to_decimal(), | ||
| }, | ||
| MakerOrderBuildError::RelVolTooLow { actual, threshold } => TradePreimageRpcError::VolumeIsTooSmall { |
There was a problem hiding this comment.
Unfortunately, it is a bit complicated to separate the TradePreimageRpcError::VolumeIsTooSmall to TradePreimageRpcError::BaseVolumeIsTooSmall and TradePreimageRpcError::RelVolumeIsTooSmall.
There was a problem hiding this comment.
I don't get why that is. Can't we just have two different errors for BaseVolumeIsTooSmall and RelVolumeIsTooSmall here instead of one
https://github.com/KomodoPlatform/atomicDEX-API/blob/30d962c392293a470f97ace63f079953f9db297c/mm2src/lp_swap/trade_preimage.rs#L213
Just to be known what is meant by the threshold because for rel volume it also depends on the price.
There was a problem hiding this comment.
Just to be known what is meant by the threshold because for rel volume it also depends on the price.
Great point! threshold is a minimum rel coin volume. The threshold price can be calculated by:
threshold_price = threshold / base_coin_volume, if method is buy
There was a problem hiding this comment.
Can't we just have two different errors
Please see where the CheckBalanceError::from_trade_preimage_error function is called. For example, here. We should pass an additional parameter indicating which coin is base and which is rel. And it will only be used to construct either BaseVolumeIsTooLow or RelVolumeIsTooLow.
But you are right, it's better to separate the TradePreimageRpcError::VolumeIsTooLow into BaseVolumeIsTooLow and RelVolumeIsTooLow, because I didn't even think how the user would calculate the threshold price.
So I think there is no problem in that some functions will have one more argument.
I'll do it tomorrow or Monday.
There was a problem hiding this comment.
CheckBalanceError::from_trade_preimage_error is called also at the start of the taker swap. But at this point, there is no information about which coin was base and which is rel. So as we discussed with @yurii-khi, it would be convenient for both MarketMaker and GUI to handle an error like
VolumeTooLow { volume: BigDecimal, threshold: BigDecimal, coin: String }. It will allow us to determine if it was a base coin volume or a rel coin volume.
There was a problem hiding this comment.
CheckBalanceError::from_trade_preimage_erroris called also at the start of the taker swap. But at this point, there is no information about which coin wasbaseand which isrel. So as we discussed with @yurii-khi, it would be convenient for both MarketMaker and GUI to handle an error like
VolumeTooLow { volume: BigDecimal, threshold: BigDecimal, coin: String }. It will allow us to determine if it was a base coin volume or a rel coin volume.
Is it possible to have some json example in case of error ? so i can start adding the data structure in cpp in my case
There was a problem hiding this comment.
@shamardy, could you please recheck the last changes?
There was a problem hiding this comment.
Is it possible to have some json example in case of error ? so i can start adding the data structure in cpp in my case
I'm working on docs right now. Please wait until I finished :)
* Rename 'TradePreimageRpcError::VolumeIsTooSmall' to 'TradePreimageRpcError::VolumeTooLow' * Add the 'coin' field to 'TradePreimageRpcError::VolumeTooLow'
