feat(trading-proto-upgrade): locked amounts, kmd burn and other impl#2046
feat(trading-proto-upgrade): locked amounts, kmd burn and other impl#2046
Conversation
Active swaps V2 RPC WIP.
Start handling require_maker_payment_confirm_before_funding_spend.
|
@laruh I added doc comments and fixed typos, thanks 🙂 |
shamardy
left a comment
There was a problem hiding this comment.
LGTM but a few non-blockers!
| // TODO tests passed without this change, need to research on how it worked | ||
| if reward.send_contract_reward_on_spend { | ||
| let eth_reward_amount = | ||
| try_tx_fus!(wei_from_big_decimal(&reward.amount, ETH_DECIMALS)); | ||
| value += eth_reward_amount; | ||
| eth_reward_amount | ||
| } else { | ||
| 0.into() | ||
| } |
There was a problem hiding this comment.
Can you please specify which tests pass? As I can see when RewardTarget is PaymentReceiver https://github.com/KomodoPlatform/komodo-defi-framework/blob/52aaa1683fc33c9c151fa44a1116142369ee63cd/mm2src/coins/utxo/utxo_common.rs#L2859
send_contract_reward_on_spend is always false
https://github.com/KomodoPlatform/komodo-defi-framework/blob/52aaa1683fc33c9c151fa44a1116142369ee63cd/mm2src/coins/utxo/utxo_common.rs#L2892
The RewardTarget::PaymentReceiver variant seems to be used for UTXO coins only for the case of UTXO taker / ETH maker - UTXO taker / ERC20 maker here #1750 (comment) this is probably why this check was not needed in eth code.
P.S. I think watchers code needs a lot of refactors to separate it from default swap case logic.
There was a problem hiding this comment.
IIRC, all watcher tests were passing with old testnet environment but started to fail after switching to dockerized Geth. send_contract_reward_on_spend can be true in get_maker_watcher_reward if other_coin.is_eth():
https://github.com/KomodoPlatform/komodo-defi-framework/blob/8635ed94a7c3334bce94c4f6d5fabbf5f227e92c/mm2src/coins/eth.rs#L2052
And when it's true, the contract attempts to send additional ETH, which isn't available on its balance. With this change, the reward ETH is also transferred to a contract during payment, making ETH/ERC20 watcher tests pass.
P.S. I think watchers code needs a lot of refactors to separate it from default swap case logic.
I agree with this. My goal was to make tests green and learn ETH watchers code/understand it better. It seems that RewardTarget and send_contract_reward_on_spend can be removed to handle most of the logic on smart contract side. I would like to either work on it myself in the future or guide the developer who will take the task.
There was a problem hiding this comment.
send_contract_reward_on_spend can be true in get_maker_watcher_reward if other_coin.is_eth()
Yeah, missed this since I was looking for cases with RewardTarget::PaymentReceive as looking for None case is not logical.
I would like to either work on it myself in the future or guide the developer who will take the task.
Sure. Thanks a lot for offering your help on this :)
| let total_payment_value = | ||
| &(&state_machine.taker_volume + &state_machine.dex_fee.total_spend_amount()) + &state_machine.taker_premium; |
There was a problem hiding this comment.
As we agreed we should include funding transaction spend fee in the total payment value of the taker. I guess you will do that in next iterations.
There was a problem hiding this comment.
Yeah, I will try to do it in the current iteration 🙂
|
@onur-ozkan @dimxy please check if your comments are resolved or not so that I can merge this PR. It will be good to have dockerized geth dev node and eth tests in dev branch asap. |
onur-ozkan
left a comment
There was a problem hiding this comment.
I did one more iteration quickly (I have no time for reviews due to my current sprint tasks), I am happy to approve the PR if everyone else approved.
|
@artemii235 can you please fix conflicts so that I can re-approve and merge this PR. |
…ration-5 # Conflicts: # Cargo.lock # mm2src/coins/eth.rs # mm2src/coins/utxo/utxo_common.rs # mm2src/mm2_bitcoin/script/src/builder.rs # mm2src/mm2_main/src/database.rs
|
@shamardy Done 🙂 |
* dev: feat(zcoin): ARRR WASM implementation (GLEECBTC#1957) feat(trading-proto-upgrade): locked amounts, kmd burn and other impl (GLEECBTC#2046) fix(indexeddb): set stop on success cursor condition (GLEECBTC#2067) feat(config): add `max_concurrent_connections` to mm2 config (GLEECBTC#2063) feat(stats_swaps): add gui/mm_version in stats db (GLEECBTC#2061) fix(indexeddb): fix IDB cursor.continue_() call after drop (GLEECBTC#2028) security bump for `h2` (GLEECBTC#2062) fix(makerbot): allow more than one prices url in makerbot (GLEECBTC#2027) fix(wasm worker env): refactor direct usage of `window` (GLEECBTC#1953) feat(nft): nft abi in withdraw_nft RPC, clear_nft_db RPC (GLEECBTC#2039) refactor(utxo): refactor utxo output script creation (GLEECBTC#1960) feat(ETH): balance event streaming for ETH (GLEECBTC#2041) chore(release): bump mm2 version to 2.1.0-beta (GLEECBTC#2044) feat(trezor): add segwit support for withdraw with trezor (GLEECBTC#1984)
#1895
What's done:
Updated deps:
docker runoptions were available).