Validate volume filter config (closes #571) #589

debnil · 2020-11-20T23:39:01Z

This PR adds additional validation for the volume filter config; calls that validation within the volume filter constructor; and adds necessary tests for that validation. It also edits other tests of the volume config that now fail because of validation. This closes #571.

nikhilsaraf

almost done

nikhilsaraf · 2020-12-02T20:00:56Z

plugins/volumeFilter.go

+
+	// return the original error, as it is already well-formatted
+	if _, e := parseVolumeFilterMode(string(c.mode)); e != nil {
+		return e


we should always wrap the error as a habit as it helps trace the error path. long term we will need to find a less verbose solution, but this has been working well so far.

plugins/volumeFilter.go

nikhilsaraf · 2020-12-02T20:10:39Z

plugins/volumeFilter_test.go

+		name             string
+		sellBaseCapBase  *float64
+		sellBaseCapQuote *float64
+		mode             volumeFilterMode


should take action as an input too.

don't need to add more than 1 test case to make sure it fails on an invalid value.
all existing cases should take a combination of buy and sell action (or maybe all are sell but only one success case is buy to prove the buy case also works)

Makes sense. To make this more efficient (vary across buy and sell for existing tests), I've just done the same loop over modes and actions we've done in other test cases.

plugins/volumeFilter.go

nikhilsaraf · 2020-12-02T20:14:16Z

plugins/volumeFilter_test.go

+		t.Run(k.name, func(t *testing.T) {
+			c := makeRawVolumeFilterConfig(k.sellBaseCapBase, k.sellBaseCapQuote, k.mode, k.marketIDs, k.accountIDs)
+			gotErr := c.Validate()
+			assert.Equal(t, gotErr, k.wantErr)


should wantErr be the second arg and gotErr the third arg?

nikhilsaraf · 2020-12-03T14:09:20Z

plugins/filterFactory.go

@@ -145,6 +145,15 @@ func makeVolumeFilterConfig(configInput string) (*VolumeFilterConfig, error) {
 		return nil, fmt.Errorf("invalid input (%s), the third part needs to be \"base\" or \"quote\"", configInput)
 	}

+	// marketIDs and account IDs might not be present in the config string, but cannot be nil on the config
+	if config.additionalMarketIDs == nil {


@debnil this is not a good idea. now we have two places that "set" these two arrays, here and in parseVolumeFilterModifier.

coding philosophy tip: the goal of software isn't to hide errors, but to surface errors that do occur more transparently, because they will occur.

in this case we are trying to hide the fact that the input value is nil by automagically handling it. What this inadvertently does is it expands the API for the VolumeFilterConfig which was not our goal here but is the unintended side-effect. please revert back.

I'm not sure if we had discussed adding this logic, but if there was a previous comment that indicated it please let me know so I can clarify requested changes better.

These lines changed because now, when we validate the config, we check that market IDs and account IDs are not nil. This function is what creates a config. Previously, when parsing a config string, if it did not specify the account and market IDs, those would be nil by default.

It wasn't explicitly requested - but this failed CI (because the config now fails validation in all of these functions). Hence, I made these changes.

The point of this pull request is to validate the config. That requires making changes like this to other functions that are now invalid. We could in theory split these in a separate PR, but that seems meaningless without the validation changes. If these changes are a bad idea, then we should either reconsider the validation scheme or change how the parsing logic works above this - I think this approach does that in the cleanest way possible, given the way that market and account IDs are parsed.

I think we need to take a step back when looking at this PR. nil is a valid value for both arrays, lets not change the API.

Can you please do the following?

In makeFilterVolume, add a comment above the line where we call utils.Dedupe to say that append(s, nil. . .) is valid.

In makeSQLQueryDailyVolume above where we check the length of optionalAccountIDs, add a comment that says that len(a) where a is a nil array is valid and returns 0

In the struct definition of VolumeFilterConfig, add a comment next to the two arrays saying that these can be nil

In TestDailyVolumeByDate_QueryRow copy the first test case and replace queryByOptionalAccountIDs: []string{} with queryByOptionalAccountIDs:nil, leaving everything else the same.

Remove all changes in this PR related to converting nil to an empty array and remove the check from validation.

If the above does not work for some reason, please let me know.

Sounds good - these are good clarifications, changes made!

nikhilsaraf · 2020-12-03T14:10:39Z

plugins/filterFactory_test.go

@@ -143,48 +143,48 @@ func TestMakeVolumeFilterConfig(t *testing.T) {
 			wantConfig: &VolumeFilterConfig{
 				BaseAssetCapInBaseUnits:  pointy.Float64(3500.0),
 				BaseAssetCapInQuoteUnits: nil,
-				additionalMarketIDs:      nil,
-				optionalAccountIDs:       nil,
+				additionalMarketIDs:      []string{},


why did these lines change?
I assume this was working correctly in the last round of reviews, otherwise I would have commented on it

PR submission tip: if this is a "new idea" you should comment on why something changed.
In general when you make a PR you should add 2-3 comments inline to "guide" the reviewer on how to view the PR.

I sent a message on Slack about this when I sent the PR, but you're right that it should get posted on the PR too. Adding it here:

I had to make a couple changes in other tests that are now failing due to validation. It doesn't really make sense to separate these changes into other PRs, since the changes matter because of validation. These are things like "no nil marketIDs or optionalAccountIDs on volumeFilterConfig after parsing".

Oh actually, I did notice these CI failures - that's why I didn't request a review on this two weeks ago - was planning on coming back and fixing those after the other PRs in buy TWAP were done, you just beat me to the review :) The version that you reviewed (the initial commit) has all of these failures.

ok thanks for clarifying. see above, we can revert all this to the original form.

nikhilsaraf

I thought we were almost done in the last review, but there's a lot of red and green lines from the last iteration of the review. Inline I've requested to revert most of the "new ideas" and to fix up the suggestions made in the previous iteration.

See the new Pull Request Guide for details on the expectations when submitting Pull Requests.

nikhilsaraf · 2020-12-03T14:23:10Z

plugins/filterFactory_test.go

@@ -143,48 +143,48 @@ func TestMakeVolumeFilterConfig(t *testing.T) {
 			wantConfig: &VolumeFilterConfig{
 				BaseAssetCapInBaseUnits:  pointy.Float64(3500.0),
 				BaseAssetCapInQuoteUnits: nil,
-				additionalMarketIDs:      nil,
-				optionalAccountIDs:       nil,
+				additionalMarketIDs:      []string{},


PR submission tip: if this is a "new idea" you should comment on why something changed.
In general when you make a PR you should add 2-3 comments inline to "guide" the reviewer on how to view the PR.

nikhilsaraf · 2020-12-03T14:27:47Z

plugins/volumeFilter_test.go

+		},
+	}
+
+	for _, mode := range []volumeFilterMode{volumeFilterModeExact, volumeFilterModeIgnore} {


the test has become more complex with this change vs what was in the last round of review, therefore harder to maintain.

there's also too many red and green lines from the last round of reviews which is increasing the PR overhead. please revert and do the changes that were requested previously, quoting below for reference since the previous comment is now marked as outdated:

should take action as an input too.

don't need to add more than 1 test case to make sure it fails on an invalid value.
all existing cases should take a combination of buy and sell action (or maybe all are sell but only one success case is buy to prove the buy case also works)

Those changes are already done. It now loops over both buy and sell actions (which is what these comments request). I'm also looping over the mode, as that's the testing pattern we've used in the tests done in the last few weeks - as I put out this PR 2-3 weeks ago, before we started varying over modes and actions in these config tests. But the mode loop is not essential, and I can hard code the mode in the testing struct if you think it makes sense.

I've also added two invalid test cases - one to test invalid action, one to test invalid mode, since those wouldn't be caught by the above testing. 2 versus 1 failure cases isn't a huge difference, especially when both test otherwise untested failure cases.

That said, this is making me reconsider the quantitative checks of red and green lines outlined in the PR policy we're discussing. The vast majority of the red and green lines in this file are because of a level of indentation changing. This is directly because of the request to test buy and sell actions for most test cases. It's just easier to do that systematically than pick some successful cases and create a couple of test cases. The additional changes due to mode are (+1, -10): one additional line to loop over the mode, and ten deletions of the mode from each test.

As discussed above, the other changes displayed are the result of separate changes that came up when I ran these tests locally, found a bunch of tests now failing because of validation, and fixed those. I'm not sure why the CI didn't trigger those failures, since validate was being called before. But I don't think not including them in this PR is an option. Agreed the process level fix here is to include a comment about this - which I sent over Slack but somehow didn't get commented to the PR - that's on me, so all good there.

It now loops over both buy and sell actions (which is what these comments request).

I think this is where the misunderstanding is on this issue. I was expecting hardcoded cases, some of which were buys and some other as sells.

let's do the following:

back out this for loop change and revert back to all the old test cases

add a new test case where it fails on an invalid value of action. success case invalid memory address or nil pointer dereference when SOURCE_SECRET_SEED is empty in config #1 should use action buy, success case Support CAP-0003: Account for the "liabilities" on an account #2 should use action sell.

I think the above is really all we need. can we do this?

Yup, perfect.

nikhilsaraf

I think we have a very clean way forward now. let's try and land this in the next shot.

nikhilsaraf · 2020-12-03T16:04:43Z

plugins/filterFactory_test.go

@@ -143,48 +143,48 @@ func TestMakeVolumeFilterConfig(t *testing.T) {
 			wantConfig: &VolumeFilterConfig{
 				BaseAssetCapInBaseUnits:  pointy.Float64(3500.0),
 				BaseAssetCapInQuoteUnits: nil,
-				additionalMarketIDs:      nil,
-				optionalAccountIDs:       nil,
+				additionalMarketIDs:      []string{},


ok thanks for clarifying. see above, we can revert all this to the original form.

nikhilsaraf · 2020-12-03T18:55:54Z

plugins/volumeFilter_test.go

+		},
+	}
+
+	for _, mode := range []volumeFilterMode{volumeFilterModeExact, volumeFilterModeIgnore} {


It now loops over both buy and sell actions (which is what these comments request).

I think this is where the misunderstanding is on this issue. I was expecting hardcoded cases, some of which were buys and some other as sells.

let's do the following:

back out this for loop change and revert back to all the old test cases

add a new test case where it fails on an invalid value of action. success case invalid memory address or nil pointer dereference when SOURCE_SECRET_SEED is empty in config #1 should use action buy, success case Support CAP-0003: Account for the "liabilities" on an account #2 should use action sell.

I think the above is really all we need. can we do this?

nikhilsaraf · 2020-12-04T07:44:33Z

plugins/filterFactory.go

@@ -145,6 +145,15 @@ func makeVolumeFilterConfig(configInput string) (*VolumeFilterConfig, error) {
 		return nil, fmt.Errorf("invalid input (%s), the third part needs to be \"base\" or \"quote\"", configInput)
 	}

+	// marketIDs and account IDs might not be present in the config string, but cannot be nil on the config
+	if config.additionalMarketIDs == nil {


I think we need to take a step back when looking at this PR. nil is a valid value for both arrays, lets not change the API.

Can you please do the following?

In makeFilterVolume, add a comment above the line where we call utils.Dedupe to say that append(s, nil. . .) is valid.

In makeSQLQueryDailyVolume above where we check the length of optionalAccountIDs, add a comment that says that len(a) where a is a nil array is valid and returns 0

In the struct definition of VolumeFilterConfig, add a comment next to the two arrays saying that these can be nil

In TestDailyVolumeByDate_QueryRow copy the first test case and replace queryByOptionalAccountIDs: []string{} with queryByOptionalAccountIDs:nil, leaving everything else the same.

Remove all changes in this PR related to converting nil to an empty array and remove the check from validation.

If the above does not work for some reason, please let me know.

nikhilsaraf · 2020-12-04T08:22:58Z

plugins/volumeFilter.go

+		return fmt.Errorf("could not parse action: %s", e)
+	}
+
+	if c.additionalMarketIDs == nil {


remove these nil errors. I think this is what is causing all the issues. these are not required since nil is a valid value for both arrays.

Got it, this makes sense and completely resolves the issue.

So just making sure we're on the same page: when validating the config, no check would be applied to market IDs or to action IDs? The only check we had discussed was non-nil. If that's now fine, I don't think another one would make sense. But let me know if we should check any values or anything like that.

nikhilsaraf

this looks great, thank you @debnil ! 🎉

Initial commit

f84586e

debnil requested a review from nikhilsaraf as a code owner November 20, 2020 23:39

Merge branch 'master' into validate-config

db804ae

nikhilsaraf reviewed Dec 2, 2020

View reviewed changes

debnil added 5 commits December 2, 2020 14:52

Merge branch 'master' into validate-config

55077fc

Merge branch 'master' into validate-config

19613d7

Vary actions and modes, check mods

8f1d7d6

Handle nil market and account IDs in other tests

5639277

Remove mode test

e410a87

nikhilsaraf reviewed Dec 3, 2020

View reviewed changes

nikhilsaraf suggested changes Dec 3, 2020

View reviewed changes

nikhilsaraf reviewed Dec 4, 2020

View reviewed changes

nikhilsaraf and others added 5 commits December 4, 2020 14:04

Merge branch 'master' into validate-config

35e2937

Revert nil checks for market and account ids.

43e4dd6

Clarify nil ids elsewhere in codebase

e0aa594

Merge branch 'master' into validate-config

01d1637

Merge branch 'master' into validate-config

b952709

nikhilsaraf approved these changes Dec 8, 2020

View reviewed changes

nikhilsaraf merged commit 48a78e0 into stellar-deprecated:master Dec 8, 2020

debnil deleted the validate-config branch December 8, 2020 22:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate volume filter config (closes #571) #589

Validate volume filter config (closes #571) #589

debnil commented Nov 20, 2020 •

edited

Loading

nikhilsaraf left a comment

nikhilsaraf Dec 2, 2020

nikhilsaraf Dec 2, 2020

debnil Dec 2, 2020

nikhilsaraf Dec 2, 2020

nikhilsaraf Dec 3, 2020 •

edited

Loading

debnil Dec 3, 2020 •

edited

Loading

nikhilsaraf Dec 4, 2020

debnil Dec 4, 2020

nikhilsaraf Dec 3, 2020 •

edited

Loading

nikhilsaraf Dec 3, 2020

debnil Dec 3, 2020

debnil Dec 3, 2020

nikhilsaraf Dec 3, 2020

nikhilsaraf left a comment

nikhilsaraf Dec 3, 2020

nikhilsaraf Dec 3, 2020

debnil Dec 3, 2020

nikhilsaraf Dec 3, 2020

debnil Dec 4, 2020

nikhilsaraf left a comment

nikhilsaraf Dec 3, 2020

nikhilsaraf Dec 3, 2020

nikhilsaraf Dec 4, 2020

nikhilsaraf Dec 4, 2020

debnil Dec 4, 2020 •

edited

Loading

nikhilsaraf left a comment

Validate volume filter config (closes #571) #589

Validate volume filter config (closes #571) #589

Conversation

debnil commented Nov 20, 2020 • edited Loading

nikhilsaraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhilsaraf Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

debnil Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhilsaraf Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhilsaraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhilsaraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debnil Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

nikhilsaraf left a comment

Choose a reason for hiding this comment

debnil commented Nov 20, 2020 •

edited

Loading

nikhilsaraf Dec 3, 2020 •

edited

Loading

debnil Dec 3, 2020 •

edited

Loading

nikhilsaraf Dec 3, 2020 •

edited

Loading

debnil Dec 4, 2020 •

edited

Loading