Skip to content

fix: bug bypassing basic filters #1217

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

Merged
merged 2 commits into from
Feb 24, 2023
Merged

fix: bug bypassing basic filters #1217

merged 2 commits into from
Feb 24, 2023

Conversation

LexLuthr
Copy link
Collaborator

@LexLuthr LexLuthr commented Feb 23, 2023

Basic filters are now working as expected.

Online = false and offline = true

Screenshot 2023-02-24 at 1 31 54 AM

online = true and offline = false
Screenshot 2023-02-24 at 1 33 42 AM

@LexLuthr LexLuthr requested a review from dirkmc February 23, 2023 20:06
Comment on lines -73 to -75
if p.config.StorageFilter == "" {
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain the change here?
I don't understand why we would make the call to get deal filter params if the StorageFilter is not set?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, it would seem we still need to call the BasicDealFilter() even when external deal filter is not set. The removed part of the code skipped that section and basic deal filter was not running. Once BasicDealFilter() completes the checks like we have enough storage, funds etc on SP side, then it runs "cmd" filter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok that makes sense, thanks 👍

In that case I would change the code in getDealFilterParams so that if the storage filter is not set, it doesn't get the funds or storage status either.

In other words at the top of getDealFilterParams:

if p.config.StorageFilter == "" {
	return &dealfilter.DealFilterParams{
                FundsState: <zero value for funds status>,
                StorageState: <zero value for storage state> ,
        }
}

Please also add a comment to the code explaining what you said in your github comment above.

@LexLuthr
Copy link
Collaborator Author

Updated the logic as per suggestion. Tested on devnet, working as expected.

Screenshot 2023-02-24 at 4 13 36 PM

@dirkmc
Copy link
Contributor

dirkmc commented Feb 24, 2023

Nice work 🙌

@LexLuthr LexLuthr merged commit a4a9bfb into main Feb 24, 2023
@LexLuthr LexLuthr deleted the fix/deal-filters branch February 24, 2023 11:07
LexLuthr added a commit that referenced this pull request Feb 24, 2023
* fix bug bypassing basic filters

* update code comments
LexLuthr added a commit that referenced this pull request Feb 24, 2023
* fix bug bypassing basic filters

* update code comments
LexLuthr added a commit that referenced this pull request Feb 24, 2023
* fix bug bypassing basic filters

* update code comments
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