fix(eth-tpu): remove state from funding validation by laruh · Pull Request #2334 · GLEECBTC/komodo-defi-framework

laruh · 2025-01-30T14:54:29Z

Note: This PRs updates eth TPU part.

This PR covers this issue #2328 in eth tpu functionality.

The payment status (PaymentSent) check has been removed from TPU validation payment functionality for all ETH implementations, including taker, maker, and maker-nft 7be74aa.
It is done to avoid unnecessary loops of checking PaymentSent in KDF code, as we already explicitly wait for taker/maker payment transaction confirmations in TPU (see wait_for_confirmations function). If tx confirmation failed, it would mean that payment doesnt have PaymentSentstate in smart contract.
Additionally this check is handled in smart contract code (see MakerPaymentState.PaymentSent in EtomicSwapMakerV2 and TakerPaymentState.PaymentSent in EtomicSwapTakerV2)
Payment state check is now only in search_for_taker_funding_spend_impl, specifically checking TakerPaymentStateV2::TakerApproved.
In TPU state machine we wait for taker payment tx confirmation (in smart contract storage it changes payment state from Unintialized to PaymentSent) before calling sign_and_broadcast_taker_payment_spend.
However, Taker payment has more states than maker payment, with the additional TakerApproved state. So we need to check somehow on KDF that taker approve tx was successful.

enum TakerPaymentState {
        Uninitialized,
        PaymentSent,
        TakerApproved,
        MakerSpent,
        TakerRefunded
    }

This TakerApproved state check is handled in search_for_taker_funding_spend function.

As maker payment has less statuses, it is sufficient to wait for maker payment tx confirmation (in smart contract storage it changes payment state from Unintialized to PaymentSent). There is no need to do additional eth calls in retry loop for this.

enum MakerPaymentState {
       Uninitialized,
       PaymentSent,
       TakerSpent,
       MakerRefunded
   }

NOTE: The below is removed, see #2334 (comment)

This PR introduces call_validate_token_with_retry, which validates in eth_taker_swap_v2.rs the payment state decoded from Token with a retry mechanism (incudes retry delay and attempts) for ETH calls.

Even though in generic state machine search_for_taker_funding_spend is already called in a loop with a timeout (see picture below), having retry logic within the ETH implementation is necessary because it directly addresses an ETH-specific issue (ref. issue #2328).

If the loop is ever removed from the state machine and ETH search_for_taker_funding_spend lacks this retry mechanism, the taker-approved check could fail, leading to a swap failure.
This can happen for "eth_call"API if the Latest block flag is used and the smart contract storage didnt get updates yet, or if the Pending flag is used and the ETH call is sent to a node that doesn't have the pending transaction.

onur-ozkan · 2025-02-03T10:10:04Z

mm2src/coins/eth.rs

+    pub(crate) async fn call_contract_function(
+        &self,
+        contract_abi: &Contract,
+        function_name: &str,


Can we use EthPaymentType to keep it pre-validated instead of using plain string?

p.s. as_str can be called inside to cast it into &str.

Can we use EthPaymentType to keep it pre-validated instead of using plain string?

It was supposed to be a common function for eth. EthPaymentType is a swap v2 specific type for payments.
I suggest to keep function_name as &str
if you dont like it we can ask for Function type in function params

and call contract_abi.function before using call_contract_function

Check this f91dfb8 please.
I think having &Function in call_contract_function even better than requiring contract and function name

Yeah that's even better

mariocynicys · 2025-02-03T11:37:34Z

Could you state what's the purpose of the PR? i.e.:

The payment status check has been removed from TPU validation payment functionality for all ETH implementations, including taker, maker, and maker-nft

Could you elaborate why?

Payment state check is now only in search_for_taker_funding_spend_impl, specifically checking TakerPaymentStateV2::TakerApproved.

And also why this stays.

mariocynicys

Thanks! Looks good with the small context I have rn.
Gonna need another review after I understand what was this for.

mm2src/coins/eth.rs

mariocynicys · 2025-02-03T11:48:01Z

mm2src/coins/eth/eth_swap_v2/eth_taker_swap_v2.rs

+            decode_index: TAKER_PAYMENT_STATE_INDEX,
+        };
+
+        let validator = |token: &Token| -> Result<bool, String> {
+            match token {
+                Token::Uint(state) => Ok(*state == U256::from(TakerPaymentStateV2::TakerApproved as u8)),
+                _ => Err(format!("Payment status must be Uint, got {:?}", token)),
+            }
+        };


the validation function seems to process only a single token at a specific index and not an array of tokens.
don't u think we can make things simpler by passing an expected value (or expected token) instead of a validation function?

hmmm, agree, I overcomplicated such trivial thing, will update it.

Wait, no, closure actually makes process of Token validation simple, as we can validate any Token enum varian with this. I tried to make call and validate functionality general for any Token variant we would like to check. Also we can create any err message in closures or use log if we need for some case.

So I added closure to have some space for validation flexibility.

About vector. I decided to not overcomplicate things by adding vector of tokens validator. As Im not sure we will ever need it. If this time come, then we will think.

as we can validate any Token enum varian with this.

we can make things simpler by passing an expected value (or expected token) instead

Also we can create any err message in closures

Well, if the errorring is strictly needed, im okay with the closure 👍

mm2src/coins/eth/eth_swap_v2/eth_taker_swap_v2.rs

laruh · 2025-02-03T15:09:21Z

Could you state what's the purpose of the PR? i.e.:

The payment status check has been removed from TPU validation payment functionality for all ETH implementations, including taker, maker, and maker-nft

Could you elaborate why?

Payment state check is now only in search_for_taker_funding_spend_impl, specifically checking TakerPaymentStateV2::TakerApproved.

And also why this stays.

I updated PR description info providing answers in sub points, please let me know if you have questions.

mariocynicys

Thanks!

The base branch was changed.

…r_payment_spend_impl, as we check TakerApproved in search_for_taker_funding_spend_impl; Provide comment related to Latest block.

…arch_for_taker_funding_spend_impl

laruh · 2025-02-08T15:07:10Z

Since the base branch was changed, I force-pushed an updated branch (cherry picked feature commits) with a clean git history to resolve conflicts

shamardy · 2025-02-17T10:54:08Z

Even though in generic state machine search_for_taker_funding_spend is already called in a loop with a timeout (see picture below), having retry logic within the ETH implementation is necessary because it directly addresses an ETH-specific issue (#2328).

About this, I have to disagree. Delayed transaction propagation or confirmation is an issue for all types of chains even for utxo but we have this implementation for utxo https://github.com/KomodoPlatform/komodo-defi-framework/blob/39515a9f3ea1089bb462e99c8cafb1049a920dbd/mm2src/coins/utxo/utxo_standard.rs#L724
search_for_taker_funding_spend searches the current state for taker funding spend, the current state definition is the queried node's current state, if the transaction is not there (or not confirmed for the EVM case as we consider the current state as the state until the last confirmed block) then it's not there currently so no need to loop inside the function.

laruh · 2025-02-17T12:55:26Z

Even though in generic state machine search_for_taker_funding_spend is already called in a loop with a timeout (see picture below), having retry logic within the ETH implementation is necessary because it directly addresses an ETH-specific issue (#2328).

About this, I have to disagree. Delayed transaction propagation or confirmation is an issue for all types of chains even for utxo but we have this implementation for utxo

https://github.com/KomodoPlatform/komodo-defi-framework/blob/39515a9f3ea1089bb462e99c8cafb1049a920dbd/mm2src/coins/utxo/utxo_standard.rs#L724

search_for_taker_funding_spend searches the current state for taker funding spend, the current state definition is the queried node's current state, if the transaction is not there (or not confirmed for the EVM case as we consider the current state as the state until the last confirmed block) then it's not there currently so no need to loop inside the function.

I see, seems like, after removing payment call from validation steps, there is no need in retry call functions (loop in state machine will be trying to get proper payment state). I will revert them in separate commit, to be able to re create them later if we need it.

laruh · 2025-02-17T13:25:37Z

@shamardy so here are three commits
358b3c1 and 7c29065 that restore payment_status_v2
and this removes retry call 77ebb20

onur-ozkan

LGTM other than some nits

mm2src/coins/eth/eth_swap_v2/eth_taker_swap_v2.rs

shamardy

Thanks for the PR! Only one comment unrelated to the PR.

shamardy · 2025-02-20T22:48:09Z

mm2src/coins/eth/eth_swap_v2/eth_taker_swap_v2.rs

Unrelated to this PR, but I noticed that the functions here are related to taker payments not taker swap, e.g. sign_and_broadcast_taker_payment_spend_impl is called by maker. Maybe we should call this file eth_taker_payments_v2 and rename eth_maker_swap_v2 to eth_maker_payment_v2 as well. You should do it in a new PR.

…yment-spend-payment-state-recall

* dev: (24 commits) fix(eth-tpu): remove state from funding validation (GLEECBTC#2334) improvement(rpc-server): rpc server dynamic port allocation (GLEECBTC#2342) fix(tests): fix or ignore unstable tests (GLEECBTC#2365) fix(fs): make `filter_files_by_extension` return only files (GLEECBTC#2364) fix(derive_key_from_path): check length of current_key_material (GLEECBTC#2356) chore(release): bump mm2 version to 2.4.0-beta (GLEECBTC#2346) fix(tests): add additional testnet sepolia nodes to test code (GLEECBTC#2358) fix(swaps): maintain legacy compatibility for negotiation messages (GLEECBTC#2353) refactor(SwapOps): impl defaults for protocol specific swapops fns (GLEECBTC#2354) feat(tpu-v2): provide swap protocol versioning (GLEECBTC#2324) feat(wallet): add change mnemonic password rpc (GLEECBTC#2317) fix(tpu-v2): fix tpu-v2 wait for payment spend and extract secret (GLEECBTC#2261) feat(tendermint): unstaking/undelegation (GLEECBTC#2330) fix(utxo-withdraw): get hw ctx only when `PrivKeyPolicy` is trezor (GLEECBTC#2333) feat(event-streaming): API-driven subscription management (GLEECBTC#2172) fix(hash-types): remove panic, enforce fixed-size arrays (GLEECBTC#2279) fix(ARRR): store unconfirmed change output (GLEECBTC#2276) feat(tendermint): staking/delegation (GLEECBTC#2322) chore(deps): `timed-map` migration (GLEECBTC#2247) fix(mem-leak): `running_swap` never shrinks (GLEECBTC#2301) ...

* dev: feat(tendermint): claim delegation rewards (#2351) fix(eth-tpu): remove state from funding validation (#2334) improvement(rpc-server): rpc server dynamic port allocation (#2342) fix(tests): fix or ignore unstable tests (#2365) fix(fs): make `filter_files_by_extension` return only files (#2364)

* dev: feat(rpc): add is_success field to legacy MySwapStatusResponse (#2371) fix(key-derivation): use stored Argon2 parameters instead of default values (#2360) fix(tests): stabilize `tendermint_coin::test_claim_staking_rewards` (#2373) improvement(RPCs): group staking rpcs under a namespace (#2372) feat(tendermint): claim delegation rewards (#2351) fix(eth-tpu): remove state from funding validation (#2334) improvement(rpc-server): rpc server dynamic port allocation (#2342) fix(tests): fix or ignore unstable tests (#2365) fix(fs): make `filter_files_by_extension` return only files (#2364) fix(derive_key_from_path): check length of current_key_material (#2356) chore(release): bump mm2 version to 2.4.0-beta (#2346) fix(tests): add additional testnet sepolia nodes to test code (#2358) fix(swaps): maintain legacy compatibility for negotiation messages (#2353) refactor(SwapOps): impl defaults for protocol specific swapops fns (#2354) feat(tpu-v2): provide swap protocol versioning (#2324) feat(wallet): add change mnemonic password rpc (#2317)

laruh added status: in progress status: pending review and removed status: in progress labels Jan 30, 2025

shamardy added the 2.4.0-beta label Feb 3, 2025

onur-ozkan reviewed Feb 3, 2025

View reviewed changes

shamardy requested a review from mariocynicys February 3, 2025 10:32

mariocynicys reviewed Feb 3, 2025

View reviewed changes

mariocynicys previously approved these changes Feb 7, 2025

View reviewed changes

Base automatically changed from fix-tpu-v2-wait-for-payment-spend to dev February 8, 2025 14:04

laruh added 15 commits February 8, 2025 21:31

delete payment state check from eth/nft TPU validation impls

128c3eb

eth-taker: delete PaymentSent state check in taker_payment_approve

c321e95

nft-maker: delete payment state check in spend_nft_maker_payment_v2_impl

466a633

call and validate token funcs with and without retry call functionality

6191d51

eth-taker: remove payment state validation in sign_and_broadcast_take…

616ff35

…r_payment_spend_impl, as we check TakerApproved in search_for_taker_funding_spend_impl; Provide comment related to Latest block.

WIP: return Result<bool, Web3RpcError> from Fn closure

2c0e8ad

delete validate_payment_state fnc

0136ea9

integrate call_validate_token_with_retry with validator closure to se…

39b8471

…arch_for_taker_funding_spend_impl

delete PaymentStatusErr and reuse PrepareTxDataError instead

70a1c53

use constants

f676b9c

return String err from validator, instead of Web3RpcError

415c24a

review: use Function in params

dea6b74

review: make consts local

4245767

review: update Latest block comment

b406a0b

review: make call_contract_function private

1e87e34

laruh force-pushed the fix-tpu-v2-wait-for-payment-spend-payment-state-recall branch from 8a67d55 to 1e87e34 Compare February 8, 2025 14:38

laruh requested a review from mariocynicys February 8, 2025 15:07

laruh requested review from onur-ozkan and removed request for mariocynicys and onur-ozkan February 8, 2025 15:07

laruh mentioned this pull request Feb 9, 2025

Trading protocol upgrade v2. #1895

Open

29 tasks

shamardy added 2.5.0-beta and removed 2.4.0-beta labels Feb 13, 2025

shamardy self-requested a review February 17, 2025 10:08

laruh added 2 commits February 17, 2025 19:47

review: return payment_status_v2

358b3c1

Retry Call was removed

77ebb20

laruh changed the title ~~fix(eth-tpu): remove state from funding validation and provide call retries~~ fix(eth-tpu): remove state from funding validation Feb 17, 2025

make payment_status_v2 private fnc

7c29065

onur-ozkan previously approved these changes Feb 19, 2025

View reviewed changes

mm2src/coins/eth/eth_swap_v2/eth_taker_swap_v2.rs Outdated Show resolved Hide resolved

mm2src/coins/eth/eth_swap_v2/eth_taker_swap_v2.rs Outdated Show resolved Hide resolved

review: update comment and avoid unnecessary variable creation

dfa01f0

laruh dismissed onur-ozkan’s stale review via dfa01f0 February 19, 2025 09:06

shamardy approved these changes Feb 20, 2025

View reviewed changes

Merge remote-tracking branch 'origin/dev' into fix-tpu-v2-wait-for-pa…

7c4fb52

…yment-spend-payment-state-recall

shamardy merged commit 490e74a into dev Feb 20, 2025

shamardy deleted the fix-tpu-v2-wait-for-payment-spend-payment-state-recall branch February 20, 2025 23:56

shamardy mentioned this pull request Apr 16, 2025

chore(release): v2.4.0-beta #2345

Merged

Conversation

laruh commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laruh Feb 3, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariocynicys commented Feb 3, 2025

Uh oh!

mariocynicys left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laruh Feb 3, 2025 • 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!

laruh Feb 3, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

laruh commented Feb 3, 2025

Uh oh!

mariocynicys left a comment

Choose a reason for hiding this comment

Uh oh!

laruh commented Feb 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shamardy commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laruh commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laruh commented Feb 17, 2025

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

laruh commented Jan 30, 2025 •

edited

Loading

laruh Feb 3, 2025 •

edited

Loading

laruh Feb 3, 2025 •

edited

Loading

laruh Feb 3, 2025 •

edited

Loading

laruh commented Feb 8, 2025 •

edited

Loading

shamardy commented Feb 17, 2025 •

edited

Loading

laruh commented Feb 17, 2025 •

edited

Loading