-
Notifications
You must be signed in to change notification settings - Fork 136
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
[CHORE] Update comparison match logic for opposite amounts #76
Conversation
Pull Request Test Coverage Report for Build 2823
💛 - Coveralls |
53f3737
to
82420b1
Compare
3533036
to
e10f1e2
Compare
parser/match_operations.go
Outdated
) | ||
} | ||
// check if all operations within the same match slice have the same amount | ||
if len(matchOps) > 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you can remove this if statement as we assume it must be true (assuming matchIndex
is not negative, which it should never be)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
equalAmounts method will return err if length of the operations is less than 1.
https://github.com/coinbase/rosetta-sdk-go/blob/master/parser/match_operations.go#L304-L306
@qiwu7 to fix the |
793bdea
to
a105399
Compare
parser/match_operations.go
Outdated
return fmt.Errorf("%w: amounts not opposites", err) | ||
} | ||
} | ||
if err := matchOperationsForOppositeComparisonValid(amountMatch[0], matches); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On further thought, I suggest we remove matchOperationsForOppositeComparisonValid
because we are just wrapping equalAmounts
with the conditionals it already contains (which I think is a bad practice).
if err := matchOperationsForOppositeComparisonValid(amountMatch[0], matches); err != nil { | |
if err := equalAmounts(matches[amountMatch[0]].Operations); err != nil { | |
return fmt.Errorf("%w: amounts not equal within index %d match operations", err, amountMatch[0]) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to surface better errors in these cases (length 0 vs length 1), I'd suggest modifying equalAmounts
itself instead of writing conditionals outside so that we don't hit this: https://github.com/coinbase/rosetta-sdk-go/blob/f929bfc4a262afbbb4053570664be00053a0b657/parser/match_operations.go#L304-L306
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we still want to allow len(ops) == 1
to be considered equal in some cases, we could add another arg that specifies if this should be allowed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still thinking if we'd want to consider operation amounts equal
if there is only 1 op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
Review Error for patrick-ogrady @ 2020-07-22 20:05:58 UTC |
@@ -301,8 +301,8 @@ func operationMatch( | |||
// equalAmounts returns an error if a slice of operations do not have | |||
// equal amounts. | |||
func equalAmounts(ops []*types.Operation) error { | |||
if len(ops) <= 1 { | |||
return fmt.Errorf("cannot check equality of %d operations", len(ops)) | |||
if len(ops) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would've expected this test to trigger a test change...can we write a simple one for testing equal amounts with 1 op? @qiwu7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually there already are test cases for equalAmounts
with only 1 op, that's why the test was failing previously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool. I can help review that when it's ready :)
Fixes # .
Motivation
Update comparison match logic for opposite amounts
Solution
use a single for loop instead of nested to simply things
Open questions