-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please specify which tests pass? As I can see when RewardTarget
is PaymentReceiver
let reward_target = RewardTarget::PaymentReceiver; |
send_contract_reward_on_spend
is always false
let send_contract_reward_on_spend = false; |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
:
komodo-defi-framework/mm2src/coins/eth.rs
Line 2052 in 8635ed9
let send_contract_reward_on_spend = other_coin.is_eth(); |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (KomodoPlatform#1957) feat(trading-proto-upgrade): locked amounts, kmd burn and other impl (KomodoPlatform#2046) fix(indexeddb): set stop on success cursor condition (KomodoPlatform#2067) feat(config): add `max_concurrent_connections` to mm2 config (KomodoPlatform#2063) feat(stats_swaps): add gui/mm_version in stats db (KomodoPlatform#2061) fix(indexeddb): fix IDB cursor.continue_() call after drop (KomodoPlatform#2028) security bump for `h2` (KomodoPlatform#2062) fix(makerbot): allow more than one prices url in makerbot (KomodoPlatform#2027) fix(wasm worker env): refactor direct usage of `window` (KomodoPlatform#1953) feat(nft): nft abi in withdraw_nft RPC, clear_nft_db RPC (KomodoPlatform#2039) refactor(utxo): refactor utxo output script creation (KomodoPlatform#1960) feat(ETH): balance event streaming for ETH (KomodoPlatform#2041) chore(release): bump mm2 version to 2.1.0-beta (KomodoPlatform#2044) feat(trezor): add segwit support for withdraw with trezor (KomodoPlatform#1984)
#1895
What's done:
Updated deps:
docker run
options were available).