[r2r] broadcast tx to txhlp / refactor tx errors by onur-ozkan · Pull Request #1245 · GLEECBTC/komodo-defi-framework

onur-ozkan · 2022-04-05T12:45:37Z

Implement P2P transaction broadcast helper. #1238
refactor TransactionFut's error type(to recover transactions in some error cases.).
write tests for functions that may return TransactionErr::TxRecoverableError
Add txhlp/TICKER P2P subscription when UTXO coins are activated in native mode.

Signed-off-by: ozkanonur <work@onurozkan.dev>

Signed-off-by: Onur Özkan <work@onurozkan.dev>

Signed-off-by: ozkanonur <work@onurozkan.dev>

artemii235

Thanks for the prompt implementation! It's the first review iteration 🙂

artemii235 · 2022-04-06T08:55:27Z

            },
        };

+        broadcast_transaction_message(


I have noticed that it's better to broadcast_transaction_message also if send_taker_refunds_payment and other fns fail. Please make send_taker_refunds_payment and others to also return the generated transaction in case of error and call broadcast_transaction_message for it.

trait SwapOps functions are (only TransactionFut returners) now calling broadcast_transaction_message right after generating transaction.

I'm a little confused 😅
Could you please tell where trait SwapOps functions call broadcast_transaction_message? I couldn't find it in the changes

I'm a little confused sweat_smile Could you please tell where trait SwapOps functions call broadcast_transaction_message? I couldn't find it in the changes

Sorry, I couldn't explain correctly. I didn't call broadcast_transaction_messageinside of the trait SwapOps functions, I call it in maker_swap and taker_swap after each trait SwapOps functions. But I guess Artem is pointing different thing than what I did here. I will update the changes today.

Signed-off-by: ozkanonur <work@onurozkan.dev>

…slp_htlc_should_fail` Signed-off-by: ozkanonur <work@onurozkan.dev>

onur-ozkan · 2022-04-07T08:38:51Z

        let err = fusd.send_raw_tx(&tx_bytes_str).wait().unwrap_err();
        println!("{:?}", err);
-        assert!(err.contains("is not valid with reason outputs greater than inputs"));
+        assert!(err.contains("the transaction was rejected by network rules.\\n\\ntransaction already in block chain"));


This had to change since send_raw_tx now calling uxto_common::send_raw_tx here @artemii235

@ozkanonur Very good that we have this test 🙂
I forgot that SLP requires additional transaction validation and provided incorrect comment that utxo_common::send_raw_tx_bytes can be used for it. Due to the missing validation, the invalid transaction was broadcast - with SLP protocol, it effectively means tokens burning (luckily, test net tokens in this case). More info on the topic: https://slp.dev/guides/slp-implementation-instructions/#have-multiple-sources-of-validation-or-client-side-validation

The usage of validation code
https://github.com/KomodoPlatform/atomicDEX-API/blob/ff6fc9f6722cf4f8ac0a290043deded201e7dc59/mm2src/coins/utxo/slp.rs#L1037-L1040

Could you please roll back the send_raw_tx implementation and use broadcast_tx in the Slp::send_raw_tx_bytes too? Please also extend this test and check that send_raw_tx_bytes will return is not valid with reason outputs greater than inputs for this tx.

Also, if the test started to fail, it's undesirable to change it - there's a very high chance that tested code got broken.

Also, if the test started to fail, it's undesirable to change it - there's a very high chance that tested code got broken.

PS, even if I asked to change the code on the review 🙂 I can be wrong.

Signed-off-by: ozkanonur <work@onurozkan.dev>

sergeyboyko0791

Great progress! Please take a look at my suggestions

Signed-off-by: ozkanonur <work@onurozkan.dev>

Signed-off-by: Onur Özkan <work@onurozkan.dev>

shamardy

I think we need to send the P2P message when broadcasting lightning-related transactions too. The message broadcasting should be added to this function https://github.com/KomodoPlatform/atomicDEX-API/blob/5c24769605a3ce7ea6c336ffff7aeec351e7705c/mm2src/coins/lightning/ln_rpc.rs#L63

onur-ozkan · 2022-04-12T11:40:59Z

I think we need to send the P2P message when broadcasting lightning-related transactions too. The message broadcasting should be added to this function

https://github.com/KomodoPlatform/atomicDEX-API/blob/5c24769605a3ce7ea6c336ffff7aeec351e7705c/mm2src/coins/lightning/ln_rpc.rs#L63

Need MmArc in order to send p2p message.

shamardy · 2022-04-12T11:47:05Z

Need MmArc in order to send p2p message.

I see, then I guess I better handle this case once this PR is merged since I changed the implementation of the BroadcasterInterface a bit in the lightning history PR. Maybe I can add P2PContext to the Platform struct that implements BroadcasterInterface as a solution, will look for the best solution then.

Signed-off-by: ozkanonur <work@onurozkan.dev>

artemii235

One change.

Signed-off-by: Onur Özkan <work@onurozkan.dev>

artemii235

I've noticed that few changes do not meet the initial requirement 🙂

Signed-off-by: ozkanonur <work@onurozkan.dev>

artemii235

One minor change

artemii235

🔥

artemii235 · 2022-04-25T12:45:23Z

Waiting for @sergeyboyko0791 approval.

sergeyboyko0791

Sorry for the late review.
It may take some time to consider my suggestions, please ping me when it's ready for the next review iteration.

sergeyboyko0791 · 2022-04-27T07:20:01Z

        Box::new(
-            self.send_to_address(address, try_fus!(wei_from_big_decimal(&amount, self.decimals)))
+            self.send_to_address(address, try_tx_fus!(wei_from_big_decimal(&amount, self.decimals)))
+                .map_err(TransactionErr::PlainError)


EthCoin::send_to_address should return TransactionErr::TxRecoverableError. Please look at the sign_and_send_transaction_impl function:
https://github.com/KomodoPlatform/atomicDEX-API/blob/c23b1f7bb2941b51297709aee3ac3c46f5370c6f/mm2src/coins/eth.rs#L1382

sergeyboyko0791 · 2022-04-27T07:24:16Z

+        let signed = try_tx_fus!(SignedEthTx::new(tx));
+        let swap_contract_address = try_tx_fus!(swap_contract_address.try_to_address());


What do you think, should we return TransactionErr::TxRecoverableError if swap_contract_address.try_to_address() fails?
Please also check if we should do that in other places.

I think we should, why not if we can just return it right?

sergeyboyko0791 · 2022-04-27T07:34:10Z


                if now_ms() / 1000 > wait_until {
-                    return ERR!(
+                    return TX_ERR!(


It seems that TX_ERR returns TransactionErr::PlainError. So what do you think about renaming it to TX_PLAIN_ERR?
Also I'd suggest using snake_case for macros. I know that we use ERR a lot, so that there is no confusion, we can leave it as SCREAMING_SNAKE_CASE. I leave it up to you :)

I agree, macros normally have snake_case naming convention, but here we are providing a compatibility for a macro that is already exists. We have to change all of them to avoid confusion(which we will do for #1247 ).

sergeyboyko0791 · 2022-04-27T07:39:07Z

+    }
+}
+
+pub type TransactionFut = Box<dyn Future<Item = TransactionEnum, Error = TransactionErr> + Send>;


Could you please consider this comment? #1258 (comment)
If you have a different point of view, I'll accept it 🙂

I think both ways are fine. Declaring on top of the module or just keep them close to it's couples. But, have to apply only one of them in the project, otherwise it will complicate things more.

@artemii235 which way we should follow? The choice will effect all the project (either all the types will move to the top, or will move to it's couple structs/functions).

sergeyboyko0791 · 2022-04-27T07:42:02Z

 }

+impl MmCoinEnum {
+    pub fn is_utxo_and_in_native_mode(&self) -> bool {


Should it be is_utxo_in_native_mode instead?

Should it be is_utxo_in_native_mode instead?

Yeah, damn what was I thinking 🤣

sergeyboyko0791 · 2022-04-27T07:46:51Z

+                return Err(TransactionErr::TxRecoverableError(
+                    TransactionEnum::from(signed),
+                    ERRL!("{:?}", err),


How about using a macro like:

/// `TransactionErr:TxRecoverableError` compatible `ERR` macro. macro_rules! TX_RECOVERABLE_ERR { ($tx: expr, $format: expr, $($args: tt)+) => { Err(crate::TransactionErr::TxRecoverableError(TransactionEnum::from($tx), ERRL!($format, $($args)+))) }; ($tx: expr, $format: expr) => { Err(crate::TransactionErr::TxRecoverableError(TransactionEnum::from($tx), ERRL!($format))) } }

This macro can be used elsewhere

Yeah that's actually very good idea

sergeyboyko0791 · 2022-04-27T07:54:24Z

@@ -1083,10 +1100,15 @@ impl MarketCoinOps for Qrc20Coin {
        from_block: u64,
        _swap_contract_address: &Option<BytesJson>,
    ) -> TransactionFut {


I think that the MarketCoinOps::wait_for_tx_spend method shouldn't return TransactionErr because it actually doesn't broadcast any transaction to the blockchain network.
I'd add another Transaction future that returns String error instead of TransactionErr.
It's only my opinion, so it's up to you to change or not

I can rename TransactionErr into TransactionFutErr to make it more general. In this case, you don't need to create another error type since we can use TransactionFutErr::Plain. Is this okay for you?

I think TransactionErr fits better, because we have functions that don't return futures.

https://github.com/KomodoPlatform/atomicDEX-API/blob/9444edabe014b1d5519e1784159f9234e87f16ce/mm2src/coins/utxo/slp.rs#L412

I'd rather have TransactionFut = Box<dyn Future<Item = TransactionEnum, Error = String>> and TxRecoverableFut = Box<dyn Future<Item = TransactionEnum, Error = TransactionFutErr>>.
What do you think?

But on the other hand, it may lead to confusion in macros, because try_tx_fus will actually return TxRecoverableFut. So, as I said, it's up to you :)

Signed-off-by: ozkanonur <work@onurozkan.dev>

onur-ozkan · 2022-04-29T12:59:53Z

@sergeyboyko0791 ready another iteration :)

Signed-off-by: ozkanonur <work@onurozkan.dev>

sergeyboyko0791

Thanks for the fixes! Second review iteration

sergeyboyko0791 · 2022-04-29T17:48:27Z

                key_pair.private().secret.as_slice(),
            );
-            let tx = try_s!(tx_fut.await);
+            let tx = try_tx_s!(tx_fut.await);


Shouldn't z_p2sh_spend return TransactionFutErr?

Sorry, I didn't understand what exactly you mean here, it's already returning TransactionFutErr.

I meant that z_p2sh_spend currently returns ZP2SHSpendError, but it could return an error extended by the TxRecoverable variant or something like that. It actually can return here
https://github.com/KomodoPlatform/atomicDEX-API/blob/5ba0dd1684ce879b4724c6c9badcbe524537f228/mm2src/coins/z_coin/z_htlc.rs#L164-L167
So maybe we can add ZP2SHSpendError::TxRecoverable 🤔

I meant that z_p2sh_spend currently returns ZP2SHSpendError, but it could return an error extended by the TxRecoverable variant or something like that. It actually can return here

https://github.com/KomodoPlatform/atomicDEX-API/blob/5ba0dd1684ce879b4724c6c9badcbe524537f228/mm2src/coins/z_coin/z_htlc.rs#L164-L167

So maybe we can add ZP2SHSpendError::TxRecoverable

Implemented it now.

@artemii235 I can rollback the commit if this isn't necessary.

sergeyboyko0791 · 2022-04-29T18:21:02Z

            Ok(Some(FoundSwapTxSpend::Spent(_))) => {
                log!("Warning: MakerPayment spent, but TakerPayment is not yet. Trying to spend TakerPayment");
                let transaction = try_s!(try_spend_taker_payment(self, &secret_hash.0).await);
+


One question to the MakerSwap::recover_funds method - should we broadcast tx message in this case?

We can't since try_spend_taker_payment doesn't' return recoverable tx.

I couldn't comment on this snippet:
https://github.com/KomodoPlatform/atomicDEX-API/blob/f87cef3ba6d7802fcb720e4a69e43c27f653e14b/mm2src/lp_swap/maker_swap.rs#L1110-L1122
So I left the link
What do you think about send_maker_spends_taker_payment?

Signed-off-by: Onur Özkan <work@onurozkan.dev>

Signed-off-by: ozkanonur <work@onurozkan.dev>

sergeyboyko0791

A few minor changes please 🙂
Also I've noticed that swap methods can be refactored with MmError, so we could avoid adding some macros. But it's overhead for this PR, so let's plan to refactor them someday later.

sergeyboyko0791 · 2022-05-03T05:45:39Z


-/// `TransactionFutErr:Plain` compatible `ERR` macro.
+/// `ZP2SHSpendError` compatible `TransactionErr` handling macro.
+macro_rules! try_ztx_s {


I think it's better to move the macro to z_coin.rs since it's compatible to ZCoin only. What do you think?

sergeyboyko0791 · 2022-05-03T05:50:39Z

+                return Err(crate::TransactionErr::Plain(format!(
+                    "{}:{}] {:?}",
+                    file!(),
+                    line!(),
+                    err


It can be simplified as:

return Err(crate::TransactionErr::Plain(ERRL!("{:?}", err)))

sergeyboyko0791 · 2022-05-03T05:58:09Z

@@ -458,11 +458,8 @@ impl Qrc20Coin {
            .generate_qrc20_transaction(outputs)
            .await
            .mm_err(|e| e.into_withdraw_error(platform, decimals))


Could you please replace mm_err with map_err here? The error trace is collected on the line below

.map_err(|e| TransactionErr::Plain(ERRL!("{}", e)))?;

So there is no need to convert the error to MmError.

Signed-off-by: Onur Özkan <work@onurozkan.dev>

onur-ozkan · 2022-05-04T07:33:01Z

A few minor changes please slightly_smiling_face Also I've noticed that swap methods can be refactored with MmError, so we could avoid adding some macros. But it's overhead for this PR, so let's plan to refactor them someday later.

I think this PR is already on the overhead stage so yeah it would be better to seperate refactoring tasks to another PR. 🙂

Fixed the notes.

sergeyboyko0791

Great! LGTM 🔥

onur-ozkan added 2 commits April 5, 2022 14:04

send p2p transaction message to txhlp/$coin.ticker topic

daeafc8

Signed-off-by: ozkanonur <work@onurozkan.dev>

propogate the message if coin inside of the topic is activated

860163c

Signed-off-by: ozkanonur <work@onurozkan.dev>

onur-ozkan requested a review from artemii235 April 5, 2022 12:45

impl send_raw_tx_bytes for MarketCoinOps

43862b6

Signed-off-by: ozkanonur <work@onurozkan.dev>

onur-ozkan force-pushed the p2p-transaction-helper-broadcast branch from 0a6d0c9 to 43862b6 Compare April 5, 2022 16:08

onur-ozkan added 5 commits April 5, 2022 19:10

update transaction broadcasting

b02ce08

Signed-off-by: ozkanonur <work@onurozkan.dev>

create and use dedicated func for sending transaction messages

f3747f7

Signed-off-by: ozkanonur <work@onurozkan.dev>

remove unused field from SwapMsgStore

7acdd35

Signed-off-by: ozkanonur <work@onurozkan.dev>

fix linting

06ecd56

Signed-off-by: Onur Özkan <work@onurozkan.dev>

log the error on transaction broadcasting

5a73d5d

Signed-off-by: ozkanonur <work@onurozkan.dev>

artemii235 suggested changes Apr 6, 2022

View reviewed changes

onur-ozkan added 5 commits April 6, 2022 12:17

fix send_raw_tx_bytes of solana

9a4c496

Signed-off-by: ozkanonur <work@onurozkan.dev>

fix send_raw_tx_bytes

4a5d9c5

Signed-off-by: ozkanonur <work@onurozkan.dev>

move use statement

a9f2e6b

Signed-off-by: ozkanonur <work@onurozkan.dev>

save development state

840d022

Signed-off-by: ozkanonur <work@onurozkan.dev>

send transactions as p2p message

658ff56

Signed-off-by: ozkanonur <work@onurozkan.dev>

onur-ozkan requested a review from artemii235 April 6, 2022 11:51

update exptected error result in test fn `construct_and_send_invalid_…

f8ed4cc

…slp_htlc_should_fail` Signed-off-by: ozkanonur <work@onurozkan.dev>

onur-ozkan commented Apr 7, 2022

View reviewed changes

onur-ozkan added 3 commits April 7, 2022 11:43

[opt] inline wrapper functions

fedbc32

Signed-off-by: ozkanonur <work@onurozkan.dev>

fix send_raw_tx and test send_raw_tx_bytes

78d64e3

Signed-off-by: ozkanonur <work@onurozkan.dev>

un-inline send_raw_tx

e9d16a9

Signed-off-by: ozkanonur <work@onurozkan.dev>

artemii235 requested a review from sergeyboyko0791 April 8, 2022 12:33

sergeyboyko0791 reviewed Apr 8, 2022

View reviewed changes

Updates after code-review

8545045

Signed-off-by: ozkanonur <work@onurozkan.dev>

artemii235 mentioned this pull request Apr 11, 2022

Implement P2P transaction broadcast helper. #1238

Open

change log level of tx broadcasting

5c24769

Signed-off-by: Onur Özkan <work@onurozkan.dev>

shamardy reviewed Apr 12, 2022

View reviewed changes

onur-ozkan changed the title ~~[r2r] broadcast tx to txhlp / refactor tx errors~~ [wip] broadcast tx to txhlp / refactor tx errors Apr 20, 2022

onur-ozkan added 2 commits April 20, 2022 15:45

refactor send_raw_tx_bytes

9c9db1c

Signed-off-by: ozkanonur <work@onurozkan.dev>

sub to txhlp/$coin when utxo coin enabled in native mode

f461dcc

Signed-off-by: ozkanonur <work@onurozkan.dev>

onur-ozkan changed the title ~~[wip] broadcast tx to txhlp / refactor tx errors~~ [r2r] broadcast tx to txhlp / refactor tx errors Apr 20, 2022

unbox TransactionEnum in TxRecoverableError

bad90b6

Signed-off-by: ozkanonur <work@onurozkan.dev>

artemii235 suggested changes Apr 21, 2022

View reviewed changes

Comment thread mm2src/rpc/lp_commands.rs Outdated

add more spesific conditions on txhlp subscription

8ca638f

Signed-off-by: Onur Özkan <work@onurozkan.dev>

artemii235 suggested changes Apr 21, 2022

View reviewed changes

rollback unnecessary broadcastings

ed574a5

Signed-off-by: ozkanonur <work@onurozkan.dev>

artemii235 suggested changes Apr 21, 2022

View reviewed changes

Comment thread mm2src/coins/solana.rs

artemii235 previously approved these changes Apr 22, 2022

View reviewed changes

sergeyboyko0791 suggested changes Apr 27, 2022

View reviewed changes

onur-ozkan changed the title ~~[r2r] broadcast tx to txhlp / refactor tx errors~~ [wip] broadcast tx to txhlp / refactor tx errors Apr 27, 2022

fix PR notes

46ad808

Signed-off-by: ozkanonur <work@onurozkan.dev>

onur-ozkan dismissed artemii235’s stale review via 46ad808 April 29, 2022 12:58

onur-ozkan changed the title ~~[wip] broadcast tx to txhlp / refactor tx errors~~ [r2r] broadcast tx to txhlp / refactor tx errors Apr 29, 2022

onur-ozkan added 3 commits April 29, 2022 16:09

fix auto-format's bug

d456326

Signed-off-by: ozkanonur <work@onurozkan.dev>

sync dev

7e6c215

remove unnecessary type casting

9444eda

sergeyboyko0791 suggested changes Apr 29, 2022

View reviewed changes

onur-ozkan added 2 commits April 30, 2022 03:44

resolve pr notes

5ba0dd1

Signed-off-by: Onur Özkan <work@onurozkan.dev>

resolve pr notes

f87cef3

Signed-off-by: ozkanonur <work@onurozkan.dev>

sergeyboyko0791 suggested changes May 3, 2022

View reviewed changes

resolve pr notes

21f9684

Signed-off-by: Onur Özkan <work@onurozkan.dev>

sergeyboyko0791 approved these changes May 4, 2022

View reviewed changes

artemii235 merged commit 9e622d5 into dev May 4, 2022

artemii235 deleted the p2p-transaction-helper-broadcast branch May 4, 2022 11:30

		let signed = try_tx_fus!(SignedEthTx::new(tx));
		let swap_contract_address = try_tx_fus!(swap_contract_address.try_to_address());

Conversation

onur-ozkan commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artemii235 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shamardy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onur-ozkan commented Apr 12, 2022

Uh oh!

shamardy commented Apr 12, 2022

Uh oh!

artemii235 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

artemii235 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

artemii235 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

artemii235 left a comment

Choose a reason for hiding this comment

Uh oh!

artemii235 commented Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

onur-ozkan commented Apr 5, 2022 •

edited

Loading

shamardy left a comment •

edited

Loading

artemii235 commented Apr 25, 2022 •

edited

Loading

onur-ozkan Apr 29, 2022 •

edited

Loading