Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(trading-proto-upgrade): locked amounts, kmd burn and other impl #2046
Changes from 58 commits
f5eb0fc
2e3f79b
8b1676b
ec6d5e2
15aca49
1f2de9b
eaf0ace
e8b99a3
0464eec
fd60519
d9ff38e
c361507
94dee63
fbf9650
6c9ab43
e1d4908
98493fb
fd53c39
a5fffe7
d684fa7
1f109f1
7bd7755
a8969b3
be9d8d9
5c7e43c
8b9fb7a
d745e73
46a91b7
222813a
a1aeb96
ee098d3
55ed473
d719246
186064c
d8dbb18
afcc59b
4cb48af
88d279b
b500c81
6b7e5ba
2efddbd
e44fc68
6bb2c27
7a0756d
1e76676
b924154
3578c17
4a5c7b2
b746028
e04ff1a
9071b1a
8369d90
79768b4
e02ed59
57352d2
c6ea8fa
52aaa16
442e759
e1b42aa
3d76e08
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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
isPaymentReceiver
komodo-defi-framework/mm2src/coins/utxo/utxo_common.rs
Line 2859 in 52aaa16
send_contract_reward_on_spend
is alwaysfalse
komodo-defi-framework/mm2src/coins/utxo/utxo_common.rs
Line 2892 in 52aaa16
The
RewardTarget::PaymentReceiver
variant seems to be used for UTXO coins only for the case ofUTXO 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 inget_maker_watcher_reward
ifother_coin.is_eth()
:komodo-defi-framework/mm2src/coins/eth.rs
Line 2052 in 8635ed9
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.
I agree with this. My goal was to make tests green and learn ETH watchers code/understand it better. It seems that
RewardTarget
andsend_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.
Yeah, missed this since I was looking for cases with
RewardTarget::PaymentReceive
as looking forNone
case is not logical.Sure. Thanks a lot for offering your help on this :)