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

Undercut option to duplicate strategy flow #878

Merged
merged 36 commits into from
Jan 18, 2024
Merged

Conversation

tiagofilipenunes
Copy link
Collaborator

@tiagofilipenunes tiagofilipenunes commented Oct 20, 2023

  • Duplicate modal
  • Logic to undercut and copy strategy
  • Adjust design to be closer to figma
    • Max modal width is lower than the widget in the figma design
    • Difference in the app button component vs figma component
    • Sheet modal increase height

Questions/additional changes:

  • When undercutting, should there be a check that the ranges might overlap and prevent it from happening? No.
  • Tooltip when hovering the duplicate option should be updated

@tiagofilipenunes tiagofilipenunes linked an issue Nov 6, 2023 that may be closed by this pull request
Copy link

cloudflare-workers-and-pages bot commented Nov 9, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: e4e5e4d
Status: ✅  Deploy successful!
Preview URL: https://2b2374be.carbon-app-csq.pages.dev
Branch Preview URL: https://issue--702.carbon-app-csq.pages.dev

View logs

@ashachaf
Copy link
Collaborator

ashachaf commented Jan 7, 2024

  • undercut diff is incorrect
    the issue is indicating a 0.1% diff, and the implementation is forcing a 1% diff.

@tiagofilipenunes tiagofilipenunes marked this pull request as ready for review January 11, 2024 17:33
Copy link
Collaborator

@GrandSchtroumpf GrandSchtroumpf left a comment

Choose a reason for hiding this comment

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

👍

src/libs/modals/modals/ModalDuplicateStrategy.tsx Outdated Show resolved Hide resolved
src/libs/modals/modals/ModalDuplicateStrategy.tsx Outdated Show resolved Hide resolved
src/libs/modals/modals/ModalDuplicateStrategy.tsx Outdated Show resolved Hide resolved
src/libs/modals/modals/ModalDuplicateStrategy.tsx Outdated Show resolved Hide resolved
@zavelevsky
Copy link
Collaborator

zavelevsky commented Jan 12, 2024

  • Add an E2E test that uses undercut option
  • Add unit tests to check the undercut formula. So you'll have to refactor const undercutStrategy = () => {} by extracting from it a pure utility function that accepts a strategy and returns a strategy - and test the numbers. Us the numbers provided in the issue.

@ashachaf
Copy link
Collaborator

ashachaf commented Jan 14, 2024

  • undercut option throws an error
image
  • undercutting limit order
    could not test, please make sure that when undercutting limit order, there is no issue with 0 price calculation.

Copy link
Collaborator

@zavelevsky zavelevsky left a comment

Choose a reason for hiding this comment

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

LGTM

@tiagofilipenunes tiagofilipenunes merged commit 49a7d7f into main Jan 18, 2024
3 of 4 checks passed
@tiagofilipenunes tiagofilipenunes deleted the issue-#702 branch January 18, 2024 12:43
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.

Add undercut option to duplicate strategy flow
4 participants