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: price discovery offers dont need to have a 0 price #946

Conversation

albertfolch-redeemeum
Copy link
Contributor

fixes: #945

@albertfolch-redeemeum albertfolch-redeemeum marked this pull request as ready for review May 14, 2024 13:04
@coveralls
Copy link

coveralls commented May 14, 2024

Coverage Status

coverage: 98.948% (+0.02%) from 98.925%
when pulling 3494523 on 945-allow-non-zero-price-discovery-buyer-cancelation-for-price-discovery-offers
into b131a97 on main.

@albertfolch-redeemeum albertfolch-redeemeum changed the title feat: price discovery offers dont need to be 0 price feat: price discovery offers dont need to have a 0 price May 17, 2024
levalleux-ludo
levalleux-ludo previously approved these changes May 22, 2024
Copy link
Member

@levalleux-ludo levalleux-ludo left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@zajck zajck left a comment

Choose a reason for hiding this comment

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

In addition to my comments in the code, can you please add a test, to see that commitToPriceDiscovery fails, if the actual price is below buyerCancelPenalty.

This must be tested for Bid, Ask and Wrapper side.

Please add them to test/protocol/PriceDiscoveryHandlerFacet.js

contracts/protocol/bases/PriceDiscoveryBase.sol Outdated Show resolved Hide resolved
test/util/utils.js Outdated Show resolved Hide resolved
test/util/utils.js Show resolved Hide resolved
test/protocol/FundsHandlerTest.js Show resolved Hide resolved
test/protocol/FundsHandlerTest.js Show resolved Hide resolved
test/protocol/FundsHandlerTest.js Outdated Show resolved Hide resolved
test/protocol/FundsHandlerTest.js Outdated Show resolved Hide resolved
test/protocol/FundsHandlerTest.js Outdated Show resolved Hide resolved
test/protocol/FundsHandlerTest.js Outdated Show resolved Hide resolved
test/protocol/FundsHandlerTest.js Outdated Show resolved Hide resolved
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
albertfolch-redeemeum and others added 11 commits May 23, 2024 11:48
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
@albertfolch-redeemeum albertfolch-redeemeum marked this pull request as draft May 23, 2024 09:51
albertfolch-redeemeum and others added 10 commits May 23, 2024 11:51
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
@albertfolch-redeemeum albertfolch-redeemeum marked this pull request as ready for review May 23, 2024 14:41
Copy link
Member

@zajck zajck left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@albertfolch-redeemeum albertfolch-redeemeum merged commit 4191e9f into main May 24, 2024
12 checks passed
@albertfolch-redeemeum albertfolch-redeemeum deleted the 945-allow-non-zero-price-discovery-buyer-cancelation-for-price-discovery-offers branch May 24, 2024 08:13
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.

Allow non-zero price discovery buyer cancelation for price discovery offers
4 participants