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(nft-swap): complete refund methods #2129
feat(nft-swap): complete refund methods #2129
Changes from 90 commits
394fe4d
44f542d
ff1006a
aebf756
1966fcf
1922bff
c07e8e2
ebac6e6
f9f4eab
ad0ce31
4a4dfc0
c703913
fe498fa
18fa53e
b9a6719
9a23f20
998708b
364d795
cb187c7
9d4e3ee
495e7a9
e917b0b
f868a50
e94fac7
2e8c662
0fbf75d
03860e1
b8b05d1
3de2b92
f2625fd
930a593
350abd6
36eaf38
4478ccb
7882e6e
d03b6d1
ff92a20
a2c47f8
d6cbfd8
9c3ff07
0bedba3
7976761
a6ad6fc
5daf723
2daec7a
6992931
3f07f1f
0079362
0c164fb
63af2bc
e2f8ccf
dad6988
9b1a700
fc4d6e4
cc8e78e
87b297a
780b1e8
f789e91
aa1d70f
b9f2a98
ece5152
920861e
e478711
9803fd5
2828be0
04d99e4
d31566d
4064999
b986279
e636da1
40ffb65
888108c
8143cc2
b391a16
5f34f1c
e672485
986c5e7
d998062
ff4d306
2ae0b04
0d54567
602206d
604d182
36d94b9
ce6673b
933d4b9
1e90600
b9049bf
8e4cd71
4e9f902
801a688
1e050d2
2447763
fa3f553
eddeb1a
7cbb941
87cc404
191b5f0
734edbc
7b69886
b13ab34
dab5bd9
cac2d0d
071a1f6
439243a
c629d7a
ed08df9
89e543f
28bc18a
69a07a8
7570df7
3042e60
e1e2242
fcd394d
6a231a3
c58808a
1a5bf73
5d6f943
2007a70
9aca0d7
fe730fa
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.
Shouldn't this use
dex_fee.total_spend_amount()
? Splitting the payment is done in the contract I think. c.c. @dimxyThere 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.
if new swap contract is r2r then we should review it in etomic swap
UPD: oh may be it is in our inner send tx functionality
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.
right now we just send dexfee to feeAddress when maker spends payment
https://github.com/KomodoPlatform/etomic-swap/blob/5e15641cbf41766cd5b37b4d71842c270773f788/contracts/EtomicSwapTakerV2.sol#L200
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.
Yep, and
total_spend_amount
will handle this in a right way. When fee splitting is implemented for EVM swaps v2, we won't have to remember changing this part.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.
@shamardy as current swap v2 contracts doesnt have burn functionality, I suggest to leave
dex_fee.fee_amount()
I will add a TODO noting the need for burnFee functionality support (added todo comment 3042e60).
The preferable approach is in wip I suppose. I would suggest to provide burnFee as an additional param in the swap contract function. And burn fee sending it to zero address right after sending
dexFee
tofeeAddress
in spend payment function.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.
Got it. I suggest to send burnFee to burnAddress to not to mix trading payments and burn fees on swap contract address.
Yep, now agree, if burnFee should be managed by
EtomicSwapTakerV2
then for eth payment I will include burn fee intoeth_total_payment
calculation.But it should be added to
eth_total_payment
when we impl burnFee managing inEtomicSwapTakerV2
, right now it doesnt have any burnFee functionality https://github.com/KomodoPlatform/etomic-swap/blob/5e15641cbf41766cd5b37b4d71842c270773f788/contracts/EtomicSwapTakerV2.solDo you want me to update taker swap contract and provide burnFee field with burnFeeAddress till Wednesday?
If you would like to refactor smart contract later I suggest not to use
total_spend_amount
asEtomicSwapTakerV2
doesnt have burnFee support.it will lead to the behaviour where we send
dex_fee
+burn_fee
asdexFee
todexFeeAddress
(in spendTakerPayment function)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.
It means that
dex_fee
will befee_amount
+burn_amount
, but we dont supportburnFee
right now. It will lead topayable(dexFeeAddress).transfer(dexFee);
as I answered in the message aboveThere 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.
It's fine by me to add it later as long as we remember to change it. I just suggested to use
total_spend_amount
now to not forget to change it later and it will be the same as fee_amount now in defi framework since we didn't add burn fee to EVM TPU in defi framework as well.No need to, we will add it at the same time we start work on EVM TPU burnfee on defi framework side.
Not really since
burn_fee
will be 0 until we support it in defi framework. And burn fee is taken from the whole fee anyways, we don't let taker pay more so that we can burn KMD, they pay the same total fee with or without burning. But as I answered above, as long as you/we remember to change it later, you can leave it as it is. Buttotal_spend_amount
was added by @onur-ozkan to use it for total fees whether we burn or not to abstract the burning part, so that other devs can use it without looking into burn fees code.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 suppose it depends on the feature. If the swap contract supports only one fee type, then its important to provide the exact fee value in the rust code. Otherwise, the current rust code may not align with the smart contract logic, even if we provide a burn fee of 0.
Its also better to handle fees separately if we agree to provide
burnFee
as a separate field in the smart contract functions.I added todo sub point in the
[FR] Implement a Fee Splitting and Burning mechanism to burn KMD and support the Komodo blockchain
issue check list #2010 (comment)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.
It does align fine since
DexFee::WithBurn
is only used forKMD
currentlykomodo-defi-framework/mm2src/mm2_main/src/lp_ordermatch.rs
Line 3147 in df6ab98
komodo-defi-framework/mm2src/mm2_main/src/lp_swap.rs
Line 817 in 6db5b9f
I am not gonna fight you more on this though :)
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.
Shouldn't we include the dex fee in the total amount to be sent?
P.S. this is how it's done for utxo
komodo-defi-framework/mm2src/coins/utxo/utxo_common.rs
Line 4791 in 4064999
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.
Hmm I see, as in
ethTakerPayment
we subtract dexfee from msg.value, then we should add dex_fee to total amount, when we sendEthCoinType::Eth
case.https://github.com/KomodoPlatform/etomic-swap/blob/5e15641cbf41766cd5b37b4d71842c270773f788/contracts/EtomicSwapTakerV2.sol#L63
In erc20 case we send
U256::from(0)
so no need to add dexfee tot total payment.But why my send-refund tests for eth passed then?
paymentHash == takerPayments[id].paymentHash
check should fail and tx should be reverted by swap contract. Will find outThere 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, only needed for Eth / platform coin case apparently. Also, for taker approval, we send dex fee separate from amount for both eth / platform coin and tokens
can you point me to this test so that I can give it a check as well.
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.
Its from second pr https://github.com/KomodoPlatform/komodo-defi-framework/pull/2169/files#diff-4bfd82ccb22215a7516f1ac3f5d1913588bee7d70ed917a3f3e5b6309b5a234a
such tests were provided
send_and_refund_taker_funding_by_secret_eth
send_and_refund_taker_funding_by_secret_erc20
send_and_refund_taker_funding_timelock_eth
send_and_refund_taker_funding_timelock_erc20
I understood why they were passing, its bcz I didnt add
wait_for_confirmations
after calling refund methods.I have
Waiting for confirmations… Failed.
issues right now using Geth node for eth and erc20https://github.com/KomodoPlatform/komodo-defi-framework/actions/runs/10057619461/job/27798973002#step:6:1513
Refund test of
Erc20
token fails confirmation waiting right onTaker sent ERC20 funding
stage. ("approve" method is used)ETH refund also has confirmation issue, but
Taker sent ETH funding
tx is confirmed, however nextTaker refunded ETH funding
txz confirmation failing.Right now Im working on sepolia deploy to see how swaps will work in this environment
log
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.
Yep, we should include dex fee to total payment for eth, will fix it.
@shamardy I understood what was the problem of failing confirmation of eth refund tests. there was a mistake in swap id code line, so tx was reverted by swap contract with
Invalid payment state. Must be PaymentSen
error https://sepolia.etherscan.io/tx/0x423762b259fbbd5667a0cc38a53a52fef062cc15289edc4a87b4dc353851b3b8I think I was confused by my previous experience of failing nft tests, when in Geth confirmations were failing, while they were working on Sepolia.
Now I can confirm that eth refund methods works (on sepolia)
https://github.com/KomodoPlatform/komodo-defi-framework/actions/runs/10075604842/job/27854240559?pr=2169#step:6:1398
I will keep working on Sepolia, as it provides the info why tx was reverted.
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.
Done c629d7a
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.
We should start using gas limits from the
gas_limit
parameter, similar to the below.komodo-defi-framework/mm2src/coins/eth.rs
Lines 3730 to 3731 in dab5bd9
We should also set constants and custom params for v2 swap functions gas limits c.c. @dimxy
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 used
self.gas_limit
in next pr #2169komodo-defi-framework/mm2src/coins/eth/eth_swap_v2.rs
Lines 80 to 83 in c924520
I will add gas limit changes into this #2129 pr as well then
UPD: done cac2d0d
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 guess we need to add new gas params for contract v2 calls (as they have different gas consumption)
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, you should add new consts and params for v2 calls @laruh. You can set the consts same as v1 for now (in this PR) until we know what are good values for them. Please add an item to your checklist about setting correct values for v2
gas_limit
consts to remember in next PRs.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.
added todo comment 7570df7
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.
Todo comments can be lost (forgotten about), we should also have it in the issue checklist, the issue checklist will be checked before closing the issue in case we forgot something.
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.
got it, added todo point here #1895 (comment)
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.
Same here