Skip to content
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

better min vol logic #2170

Merged
merged 4 commits into from
Dec 20, 2022
Merged

better min vol logic #2170

merged 4 commits into from
Dec 20, 2022

Conversation

smk762
Copy link
Collaborator

@smk762 smk762 commented Dec 7, 2022

closes #2153

To test:

  • Try to place order for/with KMD vs Smartchain at lowest amount possible.

  • Check orders tab, one side will be 0.0001, and the other side larger than 0.0001 (as below)
    image

  • Look for a pair orderbook where maker has set custom minimum, and confirm can not start swap if setting volume lower

  • Test other coin like DOGE or BTC, default minimum will be 0.1 or 0.00777 instead of 0.0001

@smk762 smk762 requested review from a user, cipig, SirSevenG and Canialon December 7, 2022 17:29
@smk762 smk762 self-assigned this Dec 7, 2022
@Canialon
Copy link
Contributor

Canialon commented Dec 7, 2022

Test other coin like DOGE or BTC, default minimum will be 0.1 or 0.00777 instead of 0.0001

take a look, default minimum volume is 0.0001, and it change to 0.1 or 0.00777 only after clicking on the custom checkbox

Screen.Recording.2022-12-07.at.21.37.18.mov

@smk762
Copy link
Collaborator Author

smk762 commented Dec 8, 2022

Thanks. It should also change when selecting an order from orderbook or setting price input (until selected price is known, it does not call https://developers.komodoplatform.com/basic-docs/atomicdex-api-legacy/min_trading_vol.html yet)

I'll see if I can add an earlier min_trading_vol call using market price to get a better approximation instead of using a static default value.

cipig
cipig previously approved these changes Dec 8, 2022
ghost
ghost previously approved these changes Dec 9, 2022
@ghost
Copy link

ghost commented Dec 9, 2022

Please test carefully and try with many different swaps with different pairs

cc. @Canialon @SirSevenG

@smk762
Copy link
Collaborator Author

smk762 commented Dec 12, 2022

Test other coin like DOGE or BTC, default minimum will be 0.1 or 0.00777 instead of 0.0001

take a look, default minimum volume is 0.0001, and it change to 0.1 or 0.00777 only after clicking on the custom checkbox

I've pushed this into a separate issue as I wont be able to fix that today and the PR is otherwise ready to review/merge.

@SirSevenG
Copy link
Contributor

Hmm

Look for a pair orderbook where maker has set custom minimum, and confirm can not start swap if setting volume lower

Generally, it works. You can't start trade with Volume < maker min_volume
Yet, there's an issue:

image

If you use exact trade volume == maker min_volume, where maker's min volume has more than 6 decimals, order will likely not match due to rounding:

image

@smk762 this PR seems to fix initial issue/report, we can also fix my problem as separate issue. What do you think?

@smk762
Copy link
Collaborator Author

smk762 commented Dec 12, 2022

Hmm

Generally, it works. You can't start trade with Volume < maker min_volume Yet, there's an issue:

image

If you use exact trade volume == maker min_volume, where maker's min volume has more than 6 decimals, order will likely not match due to rounding:

image

@smk762 this PR seems to fix initial issue/report, we can also fix my problem as separate issue. What do you think?

Yeah, def needs to be fixed - will create issue for it and tackle at same time as #2180

@SirSevenG
Copy link
Contributor

Make Error at D:/a/atomicDEX-Desktop/atomicDEX-Desktop/cmake/install/windows/windows_post_install.cmake:81 (file):
  file COPY cannot find
  "D:/a/atomicDEX-Desktop/atomicDEX-Desktop/b/bin/atomicdex-desktop.7z": File
  exists.
Call Stack (most recent call first):
  src/cmake_install.cmake:41 (include)
  cmake_install.cmake:67 (include)


 project...
FAILED: CMakeFiles/install.util 
cmd.exe /C "cd /D D:\a\atomicDEX-Desktop\atomicDEX-Desktop\b && C:\ProgramData\scoop\apps\cmake\3.22.0\bin\cmake.exe -P cmake_install.cmake"
ninja: build stopped: subcommand failed.

@smk762 @SylEze I think we've fixed same CI error previously

@smk762
Copy link
Collaborator Author

smk762 commented Dec 12, 2022

Make Error at D:/a/atomicDEX-Desktop/atomicDEX-Desktop/cmake/install/windows/windows_post_install.cmake:81 (file):
  file COPY cannot find
  "D:/a/atomicDEX-Desktop/atomicDEX-Desktop/b/bin/atomicdex-desktop.7z": File
  exists.
Call Stack (most recent call first):
  src/cmake_install.cmake:41 (include)
  cmake_install.cmake:67 (include)


 project...
FAILED: CMakeFiles/install.util 
cmd.exe /C "cd /D D:\a\atomicDEX-Desktop\atomicDEX-Desktop\b && C:\ProgramData\scoop\apps\cmake\3.22.0\bin\cmake.exe -P cmake_install.cmake"
ninja: build stopped: subcommand failed.

@smk762 @SylEze I think we've fixed same CI error previously

This one should fix it #2182

@ghost ghost requested review from cipig and a user December 12, 2022 12:20
@cipig
Copy link
Member

cipig commented Dec 12, 2022

If you use exact trade volume == maker min_volume, where maker's min volume has more than 6 decimals, order will likely not match due to rounding:

it does match, i did a lot of swaps with those orders
this is because GUI switched to using rational/fractional numbers for the volume instead of decimals some time ago, so there are no rounding errors

@smk762
Copy link
Collaborator Author

smk762 commented Dec 12, 2022

We'll need to test this more in other PR for #2183
Sometimes rounding wont be a problem depending on which way it rounds so will have to try multiple trades / valuse to make sure we have it covered

@smk762 smk762 merged commit 8494ac9 into dev Dec 20, 2022
@smk762 smk762 mentioned this pull request Mar 6, 2023
@smk762 smk762 deleted the fix_min_vol branch August 7, 2023 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]: swaps with rel/base volume < 0.00777 can't be started
4 participants