[r2r] fixes incorrect coin type for rpc error #1289

borngraced · 2022-05-20T09:27:04Z

No description provided.

sergeyboyko0791

I think that the error occurs somewhere TradePreimageError::NotSufficientBalance is constructed. Please try to figure out where exactly it happens.

sergeyboyko0791 · 2022-05-20T09:37:40Z

mm2src/lp_swap/check_balance.rs

@@ -255,7 +255,7 @@ impl CheckBalanceError {
                    }
                } else {
                    CheckBalanceError::NotSufficientBaseCoinBalance {
-                        coin,
+                        coin: ticker.to_string(),


TradePreimageError::NotSufficientBalance::coin is the coin, the balance of which is not enough.
So I think the problem is in the place where the TradePreimageError error is constructed.

For example, if coin is QRC20 and there are no QTUM enough, TradePreimageError::NotSufficientBalance::coin has to be QTUM.

sergeyboyko0791 · 2022-05-20T09:41:00Z

mm2src/lp_swap/maker_swap.rs

@@ -1865,7 +1865,7 @@ pub async fn maker_swap_trade_preimage(
    let base_coin_fee = base_coin
        .get_sender_trade_fee(preimage_value, FeeApproxStage::TradePreimage)
        .await
-        .mm_err(|e| TradePreimageRpcError::from_trade_preimage_error(e, base_coin_ticker))?;
+        .mm_err(|e| TradePreimageRpcError::from_trade_preimage_error(e, base_coin.platform_ticker()))?;


As you can see, TradePreimageRpcError::from_trade_preimage_error takes the coin ticker at this case only:

TradePreimageError::AmountIsTooSmall { amount, threshold } => CheckBalanceError::VolumeTooLow { coin: ticker.to_owned(), volume: amount, threshold, },

The ticker is needed here because TradePreimageError::AmountIsTooSmall doesn't provide it.
base_coin.platform_ticker() is not correct since we called base_coin.get_sender_trade_fee for base_coin, not for a platform coin of base_coin.

sergeyboyko0791 · 2022-05-20T09:41:06Z

mm2src/lp_swap/maker_swap.rs

@@ -1934,7 +1934,7 @@ pub async fn calc_max_maker_vol(
    let trade_fee = coin
        .get_sender_trade_fee(preimage_value, stage)
        .await
-        .mm_err(|e| CheckBalanceError::from_trade_preimage_error(e, ticker))?;
+        .mm_err(|e| CheckBalanceError::from_trade_preimage_error(e, coin.platform_ticker()))?;


shamardy

LGTM 🔥! Can you please add a unit test for this case in qrc20_tests.rs?

borngraced · 2022-05-24T13:31:04Z

LGTM 🔥! Can you please add a unit test for this case in qrc20_tests.rs?
yep!, thank you

sergeyboyko0791

Please consider my solution

sergeyboyko0791 · 2022-05-24T14:41:15Z

mm2src/coins/lp_coins.rs

@@ -1127,7 +1128,7 @@ impl TradePreimageError {
                let available = big_decimal_from_sat_unsigned(sum_utxos, decimals);
                let required = big_decimal_from_sat_unsigned(required, decimals);
                TradePreimageError::NotSufficientBalance {
-                    coin,
+                    coin: platform_ticker,


First of all, platform_ticker needs to be used instead of coin in all cases, not in GenerateTxError::NotEnoughUtxos only.
Also I think it's not really common case when a UTXO coin is a platform coin. Currently it's true for QRC20 and SLP tokens.
Summing up, I think we don't need to take platform_ticker and coin both. But we need to pass coin.platform_ticker() to the TradePreimageError:: from_generate_tx_error function instead of coin.ticker() when it's required, i.e. for QRC20 and SLP tokens.

sergeyboyko0791 · 2022-05-24T14:43:11Z

mm2src/coins/utxo/utxo_common.rs

@@ -2635,7 +2635,7 @@ pub fn get_trade_fee<T: UtxoCommonOps>(coin: T) -> Box<dyn Future<Item = TradeFe
 ///
 /// To sum up, `get_sender_trade_fee(TradePreimageValue::Exact(9000)) > get_sender_trade_fee(TradePreimageValue::Exact(10000))`.
 /// So we should always return a fee as if a transaction includes the change output.
-pub async fn preimage_trade_fee_required_to_send_outputs<T>(
+pub async fn preimage_trade_fee_required_to_send_outputs<T: MarketCoinOps>(


First, we already have trait bounds for the generic T type:

where T: UtxoCommonOps + GetUtxoListOps,

It had to be there.

Secondly, T: MarketCoinOps is not required due to the next comment

sergeyboyko0791 · 2022-05-24T14:49:06Z

mm2src/coins/utxo/utxo_common.rs

+                TradePreimageError::from_generate_tx_error(
+                    e,
+                    ticker,
+                    coin.platform_ticker().to_string(),


As I said in the first comment, TradePreimageError::from_generate_tx_error doesn't need to take platform_coin.
But we need to pass a correct ticker here. So this is what I consider as a solution:
I'd add the ticker additional argument to the utxo_common:: preimage_trade_fee_required_to_send_outputs function. The ticker argument should be used instead of
https://github.com/KomodoPlatform/atomicDEX-API/blob/4366141c8a7a5d6d3abac87df8827f7cae53d11c/mm2src/coins/utxo/utxo_common.rs#L2648

So you need to find all places where we need to pass coin.platform_ticker() instead of coin.ticker() to the utxo_common:: preimage_trade_fee_required_to_send_outputs function. And this will be a solution.
If you have other ideas, please feel free to share them to discuss 🙂

yes cool, this is also a very good solution from my side ...thoughts @shamardy

I agree with @sergeyboyko0791

…into trade_preimage-rpc-bug

…icker

shamardy

Only one comment 🙂

mm2src/mm2_main/src/docker_tests/qrc20_tests.rs

sergeyboyko0791

One change please :)

sergeyboyko0791 · 2022-05-26T10:40:09Z

mm2src/coins/utxo/bch.rs

-        utxo_common::preimage_trade_fee_required_to_send_outputs(self, outputs, fee_policy, gas_fee, stage).await
+        utxo_common::preimage_trade_fee_required_to_send_outputs(
+            self,
+            &self.ticker().to_string(),


It's better to take &str instead of &String. Then you can avoid casting self.ticker(): &str to String.

sergeyboyko0791 · 2022-05-26T14:59:07Z

Another issue related to this one...

Here we expect TradePreimageError::NotSufficientBaseCoinBalance instead of TradePreimageError::NotSufficientBalance https://github.com/KomodoPlatform/atomicDEX-API/blob/d3250f8d0c883eb377a87a56d67e1331460270c0/mm2src/coins/qrc20/qrc20_tests.rs#L742-L743

One of the possible solutions:

We try to generate a QTUM tx to send contract calls
We got GenerateTxError::NotSufficientBalance when tried to generate QTUM UTXO transaction
TradePreimageError::from_generate_tx_error must convert GenerateTxError::NotSufficientBalance to TradePreimageError::NotSufficientBaseCoinBalance when it's required like in QRC20/QTUM case

DelegationError::from_generate_tx_error, WithdrawError::from_generate_tx_error should handle this case the same way.

sergeyboyko0791

One change please :)

sergeyboyko0791 · 2022-06-01T12:29:23Z

mm2src/coins/qrc20/qrc20_tests.rs

+    let actual = block_on(coin.get_sender_trade_fee(TradePreimageValue::Exact(0.into()), FeeApproxStage::OrderIssue))
+        .err()
+        .unwrap()
+        .get_inner()
+        .to_string();
+    // expecting TradePreimageError::NotSufficientBalance
+    let expected = TradePreimageError::NotSufficientBalance {
+        coin: "tQTUM".to_string(),
+        available: BigDecimal::from_str("0").unwrap(),
+        required: BigDecimal::from_str("0.08").unwrap(),
+    };
+    assert_eq!(expected.to_string(), actual.to_string());


I'd suggest comparing two TradePreimageError errors instead of strings.
In order to do it, you need to add PartialEq to the derivings.

#1289 (comment)

Yes, it would be great! Thank you 🙂

borngraced · 2022-06-01T12:43:12Z

For the tests, I always try to follow the code style..(if similar test already exits)

Currently there's a similar test that compares PreimageError too so what if I also refactor that so they both follow a similar pattern instead of having different pattern with same outputs??

sergeyboyko0791

Great work! Thank you for the fixes!
@shamardy please review it 🙂

shamardy

Great Work 🚀 ! Only one non-blocker comment/change needed.

mm2src/coins/qrc20/qrc20_tests.rs

shamardy

Great Work 🔥!

fixes incorrect coin type for rpc error

07671d7

borngraced requested a review from sergeyboyko0791 May 20, 2022 09:45

sergeyboyko0791 suggested changes May 20, 2022

View reviewed changes

borngraced changed the title ~~fixes incorrect coin type for rpc error fixes #1275~~ [wip] fixes incorrect coin type for rpc error fixes #1275 May 20, 2022

re-impl

2949e5a

borngraced changed the title ~~[wip] fixes incorrect coin type for rpc error fixes #1275~~ [r2r] fixes incorrect coin type for rpc error fixes #1275 May 20, 2022

borngraced added 2 commits May 20, 2022 14:27

re-impl

d85117f

re-impl

08148c9

borngraced linked an issue May 23, 2022 that may be closed by this pull request

Inaccurate error message returned by trade_preimage RPC #1275

Closed

shamardy reviewed May 24, 2022

View reviewed changes

sergeyboyko0791 suggested changes May 24, 2022

View reviewed changes

borngraced added 4 commits May 25, 2022 02:12

fixes get_sender_trade_fee_preimage ticker and added unit test

18434ec

Merge branch 'dev' of https://github.com/KomodoPlatform/atomicDEX-API …

3cd7fbb

…into trade_preimage-rpc-bug

hey conflicts, I resolved yah!

3ff181f

impl unit test for TradePreimageError::NotSufficientBaseCoinBalance t…

7cfff70

…icker

shamardy reviewed May 26, 2022

View reviewed changes

mm2src/mm2_main/src/docker_tests/qrc20_tests.rs Show resolved Hide resolved

sergeyboyko0791 suggested changes May 26, 2022

View reviewed changes

minor fixes

d3250f8

borngraced changed the title ~~[r2r] fixes incorrect coin type for rpc error fixes #1275~~ [r2r] fixes incorrect coin type for rpc error May 31, 2022

sergeyboyko0791 suggested changes Jun 1, 2022

View reviewed changes

reiteration

b0adfbc

sergeyboyko0791 previously approved these changes Jun 2, 2022

View reviewed changes

shamardy previously approved these changes Jun 2, 2022

View reviewed changes

mm2src/coins/qrc20/qrc20_tests.rs Outdated Show resolved Hide resolved

ready!

614189a

borngraced dismissed stale reviews from shamardy and sergeyboyko0791 via 614189a June 2, 2022 13:58

shamardy approved these changes Jun 2, 2022

View reviewed changes

sergeyboyko0791 merged commit a499bce into dev Jun 6, 2022

sergeyboyko0791 deleted the trade_preimage-rpc-bug branch June 6, 2022 09:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[r2r] fixes incorrect coin type for rpc error #1289

[r2r] fixes incorrect coin type for rpc error #1289

borngraced commented May 20, 2022

sergeyboyko0791 left a comment

sergeyboyko0791 May 20, 2022

sergeyboyko0791 May 20, 2022

sergeyboyko0791 May 20, 2022

shamardy left a comment

borngraced commented May 24, 2022

sergeyboyko0791 left a comment

sergeyboyko0791 May 24, 2022

sergeyboyko0791 May 24, 2022

sergeyboyko0791 May 24, 2022

borngraced May 24, 2022

shamardy May 24, 2022

shamardy left a comment

sergeyboyko0791 left a comment

sergeyboyko0791 May 26, 2022

sergeyboyko0791 commented May 26, 2022 •

edited

sergeyboyko0791 left a comment

sergeyboyko0791 Jun 1, 2022

sergeyboyko0791 Jun 1, 2022

borngraced Jun 1, 2022

borngraced commented Jun 1, 2022 •

edited

sergeyboyko0791 left a comment

shamardy left a comment

shamardy left a comment

[r2r] fixes incorrect coin type for rpc error #1289

[r2r] fixes incorrect coin type for rpc error #1289

Conversation

borngraced commented May 20, 2022

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

borngraced commented May 24, 2022

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeyboyko0791 commented May 26, 2022 • edited

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borngraced commented Jun 1, 2022 • edited

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

sergeyboyko0791 commented May 26, 2022 •

edited

borngraced commented Jun 1, 2022 •

edited