Skip to content

fix(fee): remove fixed 0.0001 min threshold for TakerFee#1971

Merged
shamardy merged 8 commits intodevfrom
remove-min-threshold
Oct 3, 2023
Merged

fix(fee): remove fixed 0.0001 min threshold for TakerFee#1971
shamardy merged 8 commits intodevfrom
remove-min-threshold

Conversation

@shamardy
Copy link
Copy Markdown
Collaborator

fixes #884

Copy link
Copy Markdown

@cipig cipig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the fix, looks good

before:
image
dexfee 2.66 USD

after
image
dexfee 0.35 USD

swap worked fine
image

this was dexfee: https://polygonscan.com/tx/0x8c914293305b1eff4a2a462fd77af73467a9932b56e6187ee07e23229d56b054
0.00001325 ($0.35)

question: is this #1635 related? can we fix that too? i can't post orders with an amount lower 0.0101 WBTC-PLG20 atm, which is ~260 USD

artemii235
artemii235 previously approved these changes Sep 27, 2023
Copy link
Copy Markdown

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a great improvement!
Only one comment: dex_fee_threshold still exists in several tests' code. Is it worth renaming it to min_tx_amount for uniformity?

@shamardy
Copy link
Copy Markdown
Collaborator Author

dex_fee_threshold still exists in several tests' code. Is it worth renaming it to min_tx_amount for uniformity?

Done

@shamardy
Copy link
Copy Markdown
Collaborator Author

question: is this #1635 related? can we fix that too? i can't post orders with an amount lower 0.0101 WBTC-PLG20 atm, which is ~260 USD

It's unrelated to this PR, this is due to how minimum trading volume is calculated currently for EVM coins/tokens
https://github.com/KomodoPlatform/komodo-defi-framework/blob/51c44f67f91daec5e6f64d138c3a0abae49c9a2a/mm2src/coins/eth.rs#L2136-L2139

Since this is wrapped BTC, the decimals is 8 and the minimum is 0.01, we shouldn't depend on the decimal to calculate the minimum since what is of essence here is either the usd price or the gas price. We should work on this separately from this PR anyways, I see that @artemii235 suggested depending on the gas price in the past #873 (comment) when we didn't have the price service to get ERC20 price in relation to ETH, maybe we can look at this solution again or propose a different solution :)

artemii235
artemii235 previously approved these changes Sep 28, 2023
Copy link
Copy Markdown

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

@artemii235
Copy link
Copy Markdown

@shamardy

It's unrelated to this PR, this is due to how minimum trading volume is calculated currently for EVM coins/tokens

let pow = self.decimals / 3; looks quite strange, but it turned out that I implemented it 2 years ago 😄 Now, I think it's not justified to have such constraint because it allows e.g. 0.000001 volume, which still has little sense to use due to fees. So, it doesn't really prevent users from doing absurd trades interfering sane $260 order.

Should we maybe just use let pow = self.decimals?

@shamardy
Copy link
Copy Markdown
Collaborator Author

Should we maybe just use let pow = self.decimals?

Sure, this has no drawbacks compared to the current approach, makers get to decide the minimum volume anyways using min_base_vol. I will change this in this PR.

@shamardy
Copy link
Copy Markdown
Collaborator Author

shamardy commented Sep 28, 2023

There is also this fixed minimum price but I will leave it until we face a problem related to it https://github.com/KomodoPlatform/komodo-defi-framework/blob/8fd29ecce9f32a9fb6b3bb3221a10d66dda05a67/mm2src/mm2_main/src/lp_ordermatch.rs#L1791
@artemii235 I actually think that the validate price function should be removed completely but I want to get your opinion on this first.

@shamardy
Copy link
Copy Markdown
Collaborator Author

@cipig can you please check if this issue #1635 is fixed by the latest commits?

@cipig
Copy link
Copy Markdown

cipig commented Sep 29, 2023

@cipig can you please check if this issue #1635 is fixed by the latest commits?

It's fixed, thanks a lot.

Could trade WBTC-PLG20 worth 25 USD:
image

@artemii235
Copy link
Copy Markdown

@shamardy

I actually think that the validate price function should be removed completely but I want to get your opinion on this first.

I agree because this threshold may prevent a real trade between e.g. BTC and a very cheap asset. But it's worth checking that the price is above zero, as MmNumber can be negative.

Copy link
Copy Markdown

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor optimization 🙂

@shamardy
Copy link
Copy Markdown
Collaborator Author

shamardy commented Oct 2, 2023

But it's worth checking that the price is above zero, as MmNumber can be negative.

I kept the validate_price function but made the minimum allowed to be any value above zero.

Copy link
Copy Markdown

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@shamardy shamardy merged commit 3c078b6 into dev Oct 3, 2023
@shamardy shamardy deleted the remove-min-threshold branch October 3, 2023 10:13
shamardy added a commit that referenced this pull request Oct 11, 2023
shamardy added a commit that referenced this pull request Oct 11, 2023
…1990)

* fix test_trade_preimage_additional_validation test since price threshold is changed in #1971
* add more electrums to unstable tbch address tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

min_volume for PLG20 order can't be lower than 0.01 (0.01 WBTC ~220 USD) remove fixed 0.0001 min treshold for TakerFee

3 participants