[r2r] Mm2 error refactoring #1444

borngraced · 2022-08-16T04:08:44Z

sergeyboyko0791

Thank you for the changes!
First review iteration.
Please consider applying all comments to other coins, not ETH only.

mm2src/coins/eth.rs

sergeyboyko0791

The next review iteration

mm2src/coins/coin_errors.rs

sergeyboyko0791 · 2022-08-18T10:06:58Z

mm2src/coins/coin_errors.rs

+impl From<ScriptHashTypeNotSupported> for ValidatePaymentError {
+    fn from(err: ScriptHashTypeNotSupported) -> Self { Self::ScriptHashTypeNotSupported(err.to_string()) }
+}


Sometimes we shouldn't implement From trait for specific errors. If only you are sure that such error can be converted the same way in different contexts.
Currently, ScriptHashTypeNotSupported appears in only one case when we want to convert our QTUM address into a QRC20 contract address:
https://github.com/KomodoPlatform/atomicDEX-API/blob/18754a2017c868138387080b0dbf5ba1a1710df4/mm2src/coins/qrc20/swap.rs#L132
But what if we will need later to convert other's address into a QRC20 contract address?
ValidatePaymentError:: ScriptHashTypeNotSupported is not obvious, because the coin.validate_maker_payment() method caller wants to know about what was the reason of this error and which address has a script hash type that is not supported.

In a nutshell, I'd suggest not to implement From< ScriptHashTypeNotSupported> for ValidatePaymentError and convert it manually in the code:

let expected_receiver = qtum::contract_addr_from_utxo_addr(my_address).mm_err(|e| ValidatePaymentError::Internal(e.to_string()))?;

It's an internal error because we must be able to convert our QTUM address, otherwise it's an unexpected behaviour.

Warning

Please use the From trait carefully because later it can turn into a bug even if it's currently a correct behaviour

maybe not. I can actually use ValidatePaymentError:: ScriptHashTypeNotSupported(String) and implement From<ScriptHashTypeNotSupported> in QTUM mod alone and It won't have any of the side effects you listed above. but since it's being used here alone

let expected_receiver = qtum::contract_addr_from_utxo_addr(my_address).mm_err(|e| ValidatePaymentError::Internal(e.to_string()))?;

then I can do it manually like you said.

Implementing traits in private submodules occurs at the crate level.
For example,

struct MyFoo; struct MyBar; mod foo { use crate::{MyBar, MyFoo}; impl From<MyFoo> for MyBar { fn from(_: MyFoo) -> Self { println!("from MyFoo for MyBar"); MyBar } } } fn main() { // let x = MyFoo; let y = MyBar::from(x); }

This will work.

mm2src/coins/eth.rs

mm2src/coins/lp_coins.rs

mm2src/coins/qrc20.rs

mm2src/coins/qrc20/swap.rs

shamardy

Good start 🙂 ! First review iteration.

mm2src/coins/coin_errors.rs

mm2src/coins/eth.rs

mm2src/coins/qrc20/swap.rs

mm2src/coins/utxo/utxo_common.rs

borngraced · 2022-09-01T07:38:40Z

@shamardy @sergeyboyko0791 I have worked on the reviews and it's ready for another round of review..so I can continue with other errors. thanks)

mm2src/coins/eth.rs

sergeyboyko0791 · 2022-09-01T11:04:33Z

mm2src/coins/coin_errors.rs

+impl From<ScriptHashTypeNotSupported> for ValidatePaymentError {
+    fn from(err: ScriptHashTypeNotSupported) -> Self { Self::ScriptHashTypeNotSupported(err.to_string()) }
+}


Implementing traits in private submodules occurs at the crate level.
For example,

struct MyFoo; struct MyBar; mod foo { use crate::{MyBar, MyFoo}; impl From<MyFoo> for MyBar { fn from(_: MyFoo) -> Self { println!("from MyFoo for MyBar"); MyBar } } } fn main() { // let x = MyFoo; let y = MyBar::from(x); }

This will work.

sergeyboyko0791

Thank you for the previous fixes!
Next review iteration.

sergeyboyko0791 · 2022-09-01T11:42:48Z

mm2src/coins/eth.rs

-                    let function = try_s!(SWAP_CONTRACT.function("ethPayment"));
-                    let decoded = try_s!(function.decode_input(&tx_from_rpc.input.0));
+                    let function = SWAP_CONTRACT.function("ethPayment")?;
+                    let decoded = function.decode_input(&tx_from_rpc.input.0)?;


This is not fixed yet. Now ethabi::Error is converted into Web3RpcError. Please remove the From trait implementation and convert the error manually:

let decoded = function .decode_input(&tx_from_rpc.input.0) .map_to_mm(|e| ValidatePaymentError::InvalidPaymentTxData(e.to_string()))?;

mm2src/coins/eth.rs

mm2src/coins/lp_coins.rs

mm2src/coins/qrc20/swap.rs

mm2src/coins/utxo/qtum.rs

mm2src/coins/utxo/slp.rs

borngraced · 2022-09-02T07:28:41Z

@sergeyboyko0791 I have fixed the previous reviews and it's ready for another round:) thanks

sergeyboyko0791

Next review iteration.

mm2src/coins/eth.rs

mm2src/coins/qrc20/swap.rs

sergeyboyko0791 · 2022-09-02T10:42:42Z

mm2src/coins/utxo/bchd_grpc.rs

@@ -79,6 +79,10 @@ pub struct ValidateSlpUtxosErr {
    kind: ValidateSlpUtxosErrKind,
 }

+impl From<ValidateSlpUtxosErr> for ValidatePaymentError {
+    fn from(err: ValidateSlpUtxosErr) -> Self { Self::InvalidPaymentTxData(err.to_string()) }


We need to check what's the actual cause of the error.
For example, ValidateSlpUtxosErrKind::UnexpectedUtxoInResponse should be converted into ValidateSlpUtxosErrKind::InvalidRpcResponse, ValidateSlpUtxosErrKind::UnexpectedValidityResultType and ValidateSlpUtxosErrKind::UnexpectedTokenId are more likely InvalidRpcResponse as well (but it's better to check the code). But ValidateSlpUtxosErrKind::MultiReqErr is probably Transport error.

mm2src/coins/utxo/slp.rs

mm2src/coins/utxo/utxo_common.rs

sergeyboyko0791

We are at the finish line!
Next review iteration

mm2src/coins/eth.rs

mm2src/coins/utxo/slp.rs

mm2src/coins/utxo/utxo_common.rs

sergeyboyko0791

Only one change request please

sergeyboyko0791 · 2022-09-09T15:42:54Z

mm2src/coins/utxo/slp.rs

-            ValidateHtlcError::InvalidSlpUtxo(e) => println!("{:?}", e),
+            ValidatePaymentError::InvalidInput(e) => println!("{:?}", e),


I'm afraid that here we need to expect WrongPaymentTx, not InvalidInput. Please check if everything is correct.

sergeyboyko0791

Thank you! Approve!
@shamardy please continue reviewing it

shamardy

Final review notes/questions from my side :)

mm2src/coins/qrc20/swap.rs

mm2src/coins/utxo/bchd_grpc.rs

mm2src/coins/utxo/slp.rs

artemii235 · 2022-09-14T08:55:09Z

@sergeyboyko0791

Multiple tests expected ValidateHtlcError::InvalidSlpUtxo(ValidateSlpUtxosErr) error, but now we don't have ValidateHtlcError at all, and we need to convert ValidateSlpUtxosErr into ValidatePaymentError correctly, especially we need to determine if the UTXOs are invalid or not.

ValidateHtlcError was SLP specific, so I doubt that this should have been removed at all. The solution can be to bring it back and map ValidateHtlcError -> ValidatePaymentError.

sergeyboyko0791 · 2022-09-14T12:05:33Z

@artemii235 There was a problem with the cyclic conversion: #1444 (comment)

ValidateHtlcError was SLP specific, so I doubt that this should have been removed at all. The solution can be to bring it back and map ValidateHtlcError -> ValidatePaymentError.

artemii235 · 2022-09-14T13:28:14Z

@sergeyboyko0791

There was a problem with the cyclic conversion: #1444 (comment)

I think I have understood the problem finally and responded here: #1444 (comment) 🙂 No need to return ValidateHtlcError back, but we can refactor lower level errors a bit for clearer handling.

borngraced · 2022-09-14T14:48:10Z

@artemii235 I have updated this PR

artemii235

One last note.

artemii235 · 2022-09-15T05:03:06Z

mm2src/coins/utxo/bchd_grpc.rs

+    fn from(err: ValidateSlpUtxosErr) -> Self {
+        match err.kind {
+            ValidateSlpUtxosErrKind::MultiReqErr(_) => Self::Transport(err.to_string()),
+            ValidateSlpUtxosErrKind::InvalidSlpTxData(_) => Self::TxDeserializationError(err.to_string()),


Transaction is deserialized successfully in this case, so WrongPaymentTx seems more suitable here.

shamardy

Thank you for the fixes :)

sergeyboyko0791

Thank you for this refactoring! LGTM
@artemii235 please confirm if all change requests are solved.

# Conflicts: # mm2src/coins/eth.rs # mm2src/coins/lightning.rs # mm2src/coins/qrc20.rs # mm2src/coins/solana.rs # mm2src/coins/solana/spl.rs # mm2src/coins/tendermint/tendermint_coin.rs # mm2src/coins/test_coin.rs # mm2src/coins/utxo/bch.rs # mm2src/coins/utxo/qtum.rs # mm2src/coins/utxo/slp.rs # mm2src/coins/utxo/utxo_common.rs # mm2src/coins/utxo/utxo_standard.rs # mm2src/coins/z_coin.rs

sergeyboyko0791

Could you make last changes?

sergeyboyko0791 · 2022-09-27T14:29:29Z

mm2src/coins/utxo/utxo_common.rs

+        .map_err(|err| ValidatePaymentError::TxDeserializationError(err.to_string())));
+    let second_pub = &try_f!(Public::from_slice(&input.maker_pub)
+        .map_err(|err| ValidatePaymentError::TxDeserializationError(err.to_string())));


These two errors are InvalidInput

sergeyboyko0791

Thanks for the quick fixes! LGTM!

artemii235

👍

caglaryucekaya · 2022-10-11T12:29:12Z

The discussion is closed, but while using the ValidatePaymentError I got the impression that InvalidInput means one of the transaction inputs is invalid and only after reading #1444 (comment) I understood what it actually means, so the naming was confusing for me. How about changing it to something like InvalidParameter?

sergeyboyko0791 · 2022-10-11T14:31:06Z

It makes sense for me. Could you please rename the variant in your PR?

The discussion is closed, but while using the ValidatePaymentError I got the impression that InvalidInput means one of the transaction inputs is invalid and only after reading #1444 (comment) I understood what it actually means, so the naming was confusing for me. How about changing it to something like InvalidParameter?

borngraced added 2 commits August 16, 2022 05:01

ValidatePaymentError refactored

cf0fabd

cargo fmt

41b6db7

borngraced requested a review from sergeyboyko0791 August 16, 2022 04:08

borngraced changed the title ~~Mm2 error refactoring~~ [WIP] Mm2 error refactoring Aug 16, 2022

sergeyboyko0791 suggested changes Aug 16, 2022

View reviewed changes

borngraced added 3 commits August 16, 2022 12:52

ValidatePaymentError review fixes

d9d5717

Completed MyAddress refactoring

a01bc66

fixed WASM

18754a2

sergeyboyko0791 suggested changes Aug 18, 2022

View reviewed changes

pr review fixes for ValidatePaymentError

73ee826

shamardy requested changes Aug 29, 2022

View reviewed changes

borngraced added 2 commits August 31, 2022 10:34

ValidatePaymentError fix

3a7a31d

pr review fixes - failing tests

dd3d3b8

borngraced changed the title ~~[WIP] Mm2 error refactoring~~ [r2r] Mm2 error refactoring Sep 1, 2022

sergeyboyko0791 reviewed Sep 1, 2022

View reviewed changes

sergeyboyko0791 suggested changes Sep 1, 2022

View reviewed changes

borngraced changed the title ~~[r2r] Mm2 error refactoring~~ [wip] Mm2 error refactoring Sep 1, 2022

pr review fixes — ValidatePaymentError

5ac5c91

borngraced changed the title ~~[wip] Mm2 error refactoring~~ [r2r] Mm2 error refactoring Sep 2, 2022

sergeyboyko0791 suggested changes Sep 2, 2022

View reviewed changes

borngraced changed the title ~~[r2r] Mm2 error refactoring~~ [wip] Mm2 error refactoring Sep 2, 2022

ValidatePaymentError review fixes

2c16b1c

borngraced changed the title ~~[wip] Mm2 error refactoring~~ [r2r] Mm2 error refactoring Sep 5, 2022

sergeyboyko0791 suggested changes Sep 6, 2022

View reviewed changes

borngraced changed the title ~~[r2r] Mm2 error refactoring~~ [wip] Mm2 error refactoring Sep 6, 2022

final fix for PaymentValidationError

50cf92a

borngraced changed the title ~~[wip] Mm2 error refactoring~~ [r2r] Mm2 error refactoring Sep 7, 2022

sergeyboyko0791 suggested changes Sep 9, 2022

View reviewed changes

fix pr review notes

6ef2525

sergeyboyko0791 previously approved these changes Sep 13, 2022

View reviewed changes

shamardy reviewed Sep 13, 2022

View reviewed changes

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

mm2src/coins/utxo/bchd_grpc.rs Show resolved Hide resolved

mm2src/coins/utxo/slp.rs Outdated Show resolved Hide resolved

fix pr review notes

3719ef3

borngraced dismissed sergeyboyko0791’s stale review via 3719ef3 September 14, 2022 14:46

artemii235 requested changes Sep 15, 2022

View reviewed changes

borngraced added 2 commits September 15, 2022 11:50

review fixes

ee42a99

Merge remote-tracking branch 'origin/dev' into mm2_error_refactoring

b32c7bb

shamardy previously approved these changes Sep 15, 2022

View reviewed changes

sergeyboyko0791 previously approved these changes Sep 19, 2022

View reviewed changes

borngraced added 2 commits September 27, 2022 11:02

updated watcher_validate_taker_payment to return ValidatePaymentError

9e5f49e

borngraced dismissed stale reviews from sergeyboyko0791 and shamardy via 9e5f49e September 27, 2022 10:38

sergeyboyko0791 suggested changes Sep 27, 2022

View reviewed changes

fix InvalidInput

95b9818

sergeyboyko0791 approved these changes Sep 27, 2022

View reviewed changes

sergeyboyko0791 requested a review from artemii235 September 27, 2022 20:27

artemii235 approved these changes Sep 28, 2022

View reviewed changes

sergeyboyko0791 merged commit 5dbc5de into dev Sep 28, 2022

sergeyboyko0791 deleted the mm2_error_refactoring branch September 28, 2022 09:28

sergeyboyko0791 mentioned this pull request Oct 11, 2022

[r2r] Extend swap watcher node functionality for UTXO #1496

Merged

sergeyboyko0791 mentioned this pull request Oct 14, 2022

[r2r] Lightning taker swap p.o.c #1497

Merged

borngraced mentioned this pull request Feb 17, 2023

[release] 1.0.0-beta #1576

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[r2r] Mm2 error refactoring #1444

[r2r] Mm2 error refactoring #1444

borngraced commented Aug 16, 2022 •

edited

Loading

sergeyboyko0791 left a comment

sergeyboyko0791 left a comment

sergeyboyko0791 Aug 18, 2022

borngraced Aug 19, 2022 •

edited

Loading

sergeyboyko0791 Sep 1, 2022

shamardy left a comment

borngraced commented Sep 1, 2022

sergeyboyko0791 Sep 1, 2022

sergeyboyko0791 left a comment

sergeyboyko0791 Sep 1, 2022

borngraced commented Sep 2, 2022

sergeyboyko0791 left a comment

sergeyboyko0791 Sep 2, 2022

borngraced Sep 5, 2022

sergeyboyko0791 left a comment

sergeyboyko0791 left a comment

sergeyboyko0791 Sep 9, 2022

sergeyboyko0791 left a comment •

edited

Loading

shamardy left a comment

artemii235 commented Sep 14, 2022

sergeyboyko0791 commented Sep 14, 2022 •

edited

Loading

artemii235 commented Sep 14, 2022

borngraced commented Sep 14, 2022 •

edited

Loading

artemii235 left a comment

artemii235 Sep 15, 2022

borngraced Sep 15, 2022

shamardy left a comment

sergeyboyko0791 left a comment

sergeyboyko0791 left a comment

sergeyboyko0791 Sep 27, 2022

sergeyboyko0791 left a comment

artemii235 left a comment

caglaryucekaya commented Oct 11, 2022

sergeyboyko0791 commented Oct 11, 2022

		ValidateHtlcError::InvalidSlpUtxo(e) => println!("{:?}", e),
		ValidatePaymentError::InvalidInput(e) => println!("{:?}", e),

[r2r] Mm2 error refactoring #1444

[r2r] Mm2 error refactoring #1444

Conversation

borngraced commented Aug 16, 2022 • edited Loading

sergeyboyko0791 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

Warning

borngraced Aug 19, 2022 • edited Loading

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 Sep 1, 2022

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

borngraced commented Sep 2, 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

sergeyboyko0791 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 left a comment • edited Loading

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

artemii235 commented Sep 14, 2022

sergeyboyko0791 commented Sep 14, 2022 • edited Loading

artemii235 commented Sep 14, 2022

borngraced commented Sep 14, 2022 • edited Loading

artemii235 left a 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

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

artemii235 left a comment

Choose a reason for hiding this comment

caglaryucekaya commented Oct 11, 2022

sergeyboyko0791 commented Oct 11, 2022

borngraced commented Aug 16, 2022 •

edited

Loading

borngraced Aug 19, 2022 •

edited

Loading

sergeyboyko0791 left a comment •

edited

Loading

sergeyboyko0791 commented Sep 14, 2022 •

edited

Loading

borngraced commented Sep 14, 2022 •

edited

Loading