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

fix: Add sale time validation in FixedPriceAllowedMintersStrategy #479

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

VolodymyrBg
Copy link

Fix: Add sale time validation in FixedPriceAllowedMintersStrategy

Description

Added validation in the setSale function to ensure that saleStart is always less than saleEnd. Previously, it was possible to create a sale with invalid time window where end time was before start time, which could lead to tokens being unmintable.

Changes made:

  1. Added validation check in setSale function
  2. Added new InvalidSaleTime error to the interface
  3. Bumped contract version from 1.0.0 to 1.0.1

Motivation and Context

This fixes a potential issue where tokens could become unmintable if a sale was configured with saleEnd before saleStart. The validation ensures that all sales have a valid time window.

Does this change the ABI/API?

  • This changes the ABI/API

Added new error InvalidSaleTime to the interface. This is a backwards compatible change as it only adds a new error case.

What tests did you add/modify to account for these changes

Added test cases to verify:

  • Setting sale with valid time window succeeds
  • Setting sale with invalid time window (end before start) reverts with InvalidSaleTime
  • Setting sale with equal start and end times reverts with InvalidSaleTime

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New module / feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I added a changeset to account for this change

Reviewer Checklist:

  • My review includes a symposis of the changes and potential issues
  • The code style is enforced
  • There are no risky / concerning changes / additions to the PR

@iainnash
Copy link
Collaborator

Hello!

We do not use the fixedprice minter contracts anymore on our frontend but this is a good bug fix.

The impact of this is low as the user can update the sales configuration to a reasonable, mintable value.

This can be merged with tests.

@VolodymyrBg
Copy link
Author

@iainnash Added test

@VolodymyrBg
Copy link
Author

If the impact is low i will try to find something more significant

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.

2 participants