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

feat: minimum_balance for placing orders set to 1 atom for FOK #2558

Merged
merged 6 commits into from
Mar 25, 2024

Conversation

alfetopito
Copy link
Contributor

Description

The frontend wants to enable placement of LIMIT orders without requiring the full balance.
It can be tested here cowprotocol/cowswap#3742

It is already possible for Partially fillable orders due to the partial fill nature, but Fill or kill orders still require the full balance.

The arguments for the feature is:

  1. allow users setting up a limit order in advance of receiving funds, for example in a newly created Safe. Once funds are received, order becomes valid and trade-able

The argument to remove the Fill or kill limitation is:

  1. allow users a seamless experience when placing LIMIT orders across both sub-types, without requiring a different balance constraint

Due to spam protection, 1 atom of the sell token balance is still required, and that is not being removed.

Changes

  • Remove full balance limitation for both order types (Fill or kill and Partially fillable)
  • Turn minimum_balance function into a constant of value 1.

How to test

This change relies on existing unit tests

Notes

Please do let me know where best to put the constant, whether there is need to add any specific unit test and so on

@alfetopito alfetopito requested a review from a team as a code owner March 21, 2024 15:43
@alfetopito alfetopito self-assigned this Mar 21, 2024
@alfetopito alfetopito force-pushed the feat/remove-full-balance-requirement-for-fok branch from d8f12b7 to 8cf01f7 Compare March 21, 2024 15:52
Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

An e2e test would be nice:

  1. create order where user only has 1 wei of the sell token
  2. fund the user account
  3. await order getting settled

But I can also add that to the PR if you like.

@alfetopito
Copy link
Contributor Author

An e2e test would be nice:

1. create order where user only has 1 wei of the sell token

2. fund the user account

3. await order getting settled

That's way beyond my current capacity and patience levels

But I can also add that to the PR if you like.

Pretty please? 🥺

@alfetopito alfetopito force-pushed the feat/remove-full-balance-requirement-for-fok branch from 69608d2 to 5ed9d35 Compare March 22, 2024 11:47
alfetopito and others added 2 commits March 25, 2024 10:43
Co-authored-by: Martin Beckmann <martin.beckmann@protonmail.com>
@alfetopito alfetopito force-pushed the feat/remove-full-balance-requirement-for-fok branch from 5ed9d35 to 6badecb Compare March 25, 2024 10:43
Copy link
Contributor

@squadgazzz squadgazzz left a comment

Choose a reason for hiding this comment

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

LG

@MartinquaXD MartinquaXD enabled auto-merge (squash) March 25, 2024 14:53
@MartinquaXD MartinquaXD merged commit 5950d92 into main Mar 25, 2024
9 checks passed
@MartinquaXD MartinquaXD deleted the feat/remove-full-balance-requirement-for-fok branch March 25, 2024 15:00
@github-actions github-actions bot locked and limited conversation to collaborators Mar 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants