[r2r] Tx wait for confirmation timeout fix #1446

borngraced · 2022-08-17T09:19:45Z

shamardy

Good progress! I added some comments about what we discussed in discord :)
You will need to refactor more code other than what I mentioned in the below comments since I only highlighted the most important changes here.

mm2src/common/jsonrpc_client.rs

mm2src/coins/utxo/rpc_clients.rs

mm2src/common/jsonrpc_client.rs

mm2src/mm2_main/src/docker_tests.rs

mm2src/coins/utxo/rpc_clients.rs

shamardy

Second review iteration. Next time, please revise the whole PR changes before making it ready to review :)

mm2src/coins/lp_coins.rs

mm2src/coins/qrc20/history.rs

mm2src/coins/utxo.rs

mm2src/coins/utxo/rpc_clients.rs

mm2src/coins/utxo/slp.rs

mm2src/coins/z_coin/z_coin_errors.rs

mm2src/common/jsonrpc_client.rs

borngraced · 2022-08-29T07:41:17Z

@shamardy this is now ready for another round of review. thanks

shamardy

Third review iteration :)

mm2src/common/jsonrpc_client.rs

mm2src/coins/utxo/rpc_clients.rs

borngraced · 2022-08-30T11:37:07Z

@shamardy this is now ready for another round of review. thanks)

sergeyboyko0791 · 2022-09-02T12:23:40Z

@borngraced could you please check if it's possible to compile NativeClient and everything that works with it to a #[cfg(not(target_arch = "wasm32"))]?
But please stop at the moment if you realize that this refactoring will require a lot of effort.

This could help us to avoid JsonRpcErrorType::Internal error variant.

shamardy · 2022-09-02T13:26:48Z

This could help us to avoid JsonRpcErrorType::Internal error variant.

After some discussion with @sergeyboyko0791, we decided that JsonRpcErrorType::Internal error variant should be kept because of the mapping from SlurpError::Internal.

@borngraced could you please check if it's possible to compile NativeClient and everything that works with it to a #[cfg(not(target_arch = "wasm32"))]?
But please stop at the moment if you realize that this refactoring will require a lot of effort.

Also, this can be done later on a seperate PR, it is not required now.

borngraced · 2022-09-02T13:45:32Z

This could help us to avoid JsonRpcErrorType::Internal error variant.

After some discussion with @sergeyboyko0791, we decided that JsonRpcErrorType::Internal error variant should be kept because of the mapping from SlurpError::Internal.

@borngraced could you please check if it's possible to compile NativeClient and everything that works with it to a #[cfg(not(target_arch = "wasm32"))]?
But please stop at the moment if you realize that this refactoring will require a lot of effort.

Also, this can be done later on a seperate PR, it is not required now.

alright noted. 👍

shamardy

Good progress! Another review round :)

mm2src/coins/utxo/rpc_clients.rs

mm2src/common/jsonrpc_client.rs

mm2src/coins/utxo/rpc_clients.rs

mm2src/mm2_net/src/transport.rs

shamardy

Only 2 comments. Last review iteration from my side if those are resolved without issues :)

mm2src/coins/utxo/rpc_clients.rs

borngraced · 2022-09-06T10:52:41Z

@shamardy @sergeyboyko0791 this is ready for another round of review.

shamardy

Great Work! Only some minor changes left!

mm2src/coins/utxo/rpc_clients.rs

mm2src/common/executor/wasm_executor.rs

mm2src/common/jsonrpc_client.rs

mm2src/coins/utxo/rpc_clients.rs

mm2src/common/jsonrpc_client.rs

shamardy · 2022-09-06T17:09:55Z

@borngraced please also fix test_electrum_rpc_client_error

shamardy · 2022-09-06T17:16:25Z

@borngraced please also fix test_electrum_rpc_client_error

Nevermind, it will be fixed automatically by this #1446 (comment)

shamardy

LGTM!

sergeyboyko0791

Great refactoring! We're getting rid of the String errors step-by-step.
Please consider my change requests.

mm2src/coins/utxo/rpc_clients.rs

mm2src/coins/utxo/utxo_common.rs

mm2src/coins/utxo/utxo_tests.rs

mm2src/common/jsonrpc_client.rs

mm2src/mm2_core/src/mm_ctx.rs

mm2src/mm2_main/src/docker_tests.rs

sergeyboyko0791

Thank you for this enhancement!
Approve 🔥

…rror_fix

…rror_fix # Conflicts: # mm2src/coins/utxo/utxo_indexedb_block_header_storage.rs # mm2src/mm2_core/src/mm_ctx.rs

shamardy

Re-approve

…rror_fix

sergeyboyko0791

Thank you for the great enhancement!
Approve!

borngraced requested a review from shamardy August 17, 2022 09:19

shamardy requested changes Aug 22, 2022

View reviewed changes

shamardy reviewed Aug 26, 2022

View reviewed changes

borngraced changed the title ~~[r2r] Tx wait for confirmation timeout fix~~ [WIP] Tx wait for confirmation timeout fix Aug 26, 2022

borngraced changed the title ~~[WIP] Tx wait for confirmation timeout fix~~ [r2r] Tx wait for confirmation timeout fix Aug 29, 2022

shamardy requested changes Aug 29, 2022

View reviewed changes

borngraced changed the title ~~[r2r] Tx wait for confirmation timeout fix~~ [wip] Tx wait for confirmation timeout fix Aug 30, 2022

borngraced changed the title ~~[wip] Tx wait for confirmation timeout fix~~ [r2r] Tx wait for confirmation timeout fix Aug 30, 2022

borngraced changed the title ~~[r2r] Tx wait for confirmation timeout fix~~ [wip] Tx wait for confirmation timeout fix Sep 2, 2022

shamardy requested changes Sep 2, 2022

View reviewed changes

borngraced changed the title ~~[wip] Tx wait for confirmation timeout fix~~ [r2r] Tx wait for confirmation timeout fix Sep 5, 2022

shamardy requested changes Sep 5, 2022

View reviewed changes

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

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

borngraced changed the title ~~[r2r] Tx wait for confirmation timeout fix~~ [wip] Tx wait for confirmation timeout fix Sep 6, 2022

borngraced changed the title ~~[wip] Tx wait for confirmation timeout fix~~ [r2r] Tx wait for confirmation timeout fix Sep 6, 2022

borngraced changed the title ~~[r2r] Tx wait for confirmation timeout fix~~ [wip] Tx wait for confirmation timeout fix Sep 6, 2022

borngraced changed the title ~~[wip] Tx wait for confirmation timeout fix~~ [r2r] Tx wait for confirmation timeout fix Sep 6, 2022

shamardy requested changes Sep 6, 2022

View reviewed changes

shamardy previously approved these changes Sep 7, 2022

View reviewed changes

sergeyboyko0791 suggested changes Sep 7, 2022

View reviewed changes

borngraced dismissed shamardy’s stale review via 580c717 September 7, 2022 17:34

sergeyboyko0791 previously approved these changes Sep 9, 2022

View reviewed changes

borngraced dismissed sergeyboyko0791’s stale review via 34b6297 September 9, 2022 15:47

borngraced added 17 commits September 11, 2022 20:29

electrum_request error refactor

4d43e61

PR review fixes

ca0023e

review fixes — moved InvalidRequest Error from electrum_request fn

62e4bf8

pr review fixes, fixed wasm warnings

eb68d42

pr review fixes

9c2dfae

review fixes

8c1a840

review fixes

cfee27b

fixed last review note

3d8028c

fix electrum and nattive tests

dcb88be

comments

f31c4d6

improved unit tests

5150c97

fix — test_for_non_existent_tx_hex_utxo

511af7d

test debugging on ci build

83b3f5b

test debugging for ci build

dae8d7d

fmt

1bcbdd9

ci build debug

7dc8040

fixed unit teat error

bd55559

borngraced force-pushed the tx_wait_confirmation_error_fix branch from 252776d to bd55559 Compare September 12, 2022 10:38

shamardy mentioned this pull request Sep 12, 2022

[r2r] Swap watcher node basic functionality #1457

Merged

borngraced added 5 commits September 12, 2022 12:20

Merge remote-tracking branch 'origin/dev' into tx_wait_confirmation_e…

abc2d3a

…rror_fix

Merge remote-tracking branch 'origin/dev' into tx_wait_confirmation_e…

6bfa3e3

…rror_fix # Conflicts: # mm2src/coins/utxo/utxo_indexedb_block_header_storage.rs # mm2src/mm2_core/src/mm_ctx.rs

cargo fmt

d3ad8c9

cargo fmt

79eaf39

fix mm_ctx imports

1d43ca8

shamardy approved these changes Sep 12, 2022

View reviewed changes

Merge remote-tracking branch 'origin/dev' into tx_wait_confirmation_e…

1293884

…rror_fix

sergeyboyko0791 approved these changes Sep 13, 2022

View reviewed changes

sergeyboyko0791 merged commit 1438bf9 into dev Sep 13, 2022

sergeyboyko0791 deleted the tx_wait_confirmation_error_fix branch September 13, 2022 12:23

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] Tx wait for confirmation timeout fix #1446

[r2r] Tx wait for confirmation timeout fix #1446

borngraced commented Aug 17, 2022

shamardy left a comment

shamardy left a comment

borngraced commented Aug 29, 2022

shamardy left a comment

borngraced commented Aug 30, 2022

sergeyboyko0791 commented Sep 2, 2022

shamardy commented Sep 2, 2022

borngraced commented Sep 2, 2022

shamardy left a comment

shamardy left a comment

borngraced commented Sep 6, 2022

shamardy left a comment

shamardy commented Sep 6, 2022

shamardy commented Sep 6, 2022

shamardy left a comment

sergeyboyko0791 left a comment

sergeyboyko0791 left a comment

shamardy left a comment

sergeyboyko0791 left a comment

[r2r] Tx wait for confirmation timeout fix #1446

[r2r] Tx wait for confirmation timeout fix #1446

Conversation

borngraced commented Aug 17, 2022

shamardy left a comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

borngraced commented Aug 29, 2022

shamardy left a comment

Choose a reason for hiding this comment

borngraced commented Aug 30, 2022

sergeyboyko0791 commented Sep 2, 2022

shamardy commented Sep 2, 2022

borngraced commented Sep 2, 2022

shamardy left a comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

borngraced commented Sep 6, 2022

shamardy left a comment

Choose a reason for hiding this comment

shamardy commented Sep 6, 2022

shamardy commented Sep 6, 2022

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

shamardy left a comment

Choose a reason for hiding this comment

sergeyboyko0791 left a comment

Choose a reason for hiding this comment