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

[Overlapping] Enable undercut #1357

Merged
merged 19 commits into from
Jul 18, 2024
Merged

[Overlapping] Enable undercut #1357

merged 19 commits into from
Jul 18, 2024

Conversation

GrandSchtroumpf
Copy link
Collaborator

fix #1325

Enable undercut for overlapping strategy
Reduce spread by 0.1%.

Test cases :

  1. Create an overlapping strategy with spread of 1%
  2. Menu -> Duplicate -> Undercut
  3. Spread is now 0.99%

This work if strategy :

  • has no budget nor market price
  • is above/below market price
  • are built out of a recurring strategy (default to 0.05*0.99)

Copy link

cloudflare-workers-and-pages bot commented Jul 16, 2024

Deploying carbon-app-sei with  Cloudflare Pages  Cloudflare Pages

Latest commit: ed99aba
Status: ✅  Deploy successful!
Preview URL: https://1a145ed6.carbon-app-sei.pages.dev
Branch Preview URL: https://issue--1325.carbon-app-sei.pages.dev

View logs

Copy link

cloudflare-workers-and-pages bot commented Jul 16, 2024

Deploying carbon-app with  Cloudflare Pages  Cloudflare Pages

Latest commit: ed99aba
Status: ✅  Deploy successful!
Preview URL: https://aebeef6a.carbon-app-csq.pages.dev
Branch Preview URL: https://issue--1325.carbon-app-csq.pages.dev

View logs

Copy link
Collaborator

@tiagofilipenunes tiagofilipenunes left a comment

Choose a reason for hiding this comment

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

LGTM

Only thing to note is if the overlapping strategy has a defined budget, this budget (and anchor) won't be populated when undercutting, which is the same behaviour as overlapping duplicate but different from recurring/disposable undercut/duplicate.

@ashachaf
Copy link
Collaborator

ashachaf commented Jul 16, 2024

Copy link
Collaborator

@tiagofilipenunes tiagofilipenunes left a comment

Choose a reason for hiding this comment

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

LGTM just one change request for the e2e test

  1. Currently, the create/duplicate/undercut tests all override the create/form.png screenshot. Can you change the submit method of CreateStrategyDriver to something like this
  async submit(type: StrategyCase) {
    const btn = this.page.getByText('Create Strategy');
    if (shouldTakeScreenshot) {
      const mainMenu = new MainMenuDriver(this.page);
      await mainMenu.hide();
      await waitTooltipsClose(this.page);
      const form = this.getForm();
      const path = screenshotPath(this.testCase, type, 'form');
      await screenshot(form, path);
      await mainMenu.show();
    }
    try {
      await waitFor(this.page, 'approve-warnings', 2_000);
      if (await this.page.isVisible('[data-testid=approve-warnings]')) {
        await this.page.getByTestId('approve-warnings').click();
      }
      await btn.click();
    } catch {
      await btn.click({ timeout: 1_000 });
    }
  }

and then set they type to the strategy type.

@GrandSchtroumpf GrandSchtroumpf merged commit aba5213 into main Jul 18, 2024
2 checks passed
@GrandSchtroumpf GrandSchtroumpf deleted the issue-#1325 branch July 18, 2024 08:11
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.

Strategy duplicate - expose undercut option for concentrated strategy
3 participants