-
Notifications
You must be signed in to change notification settings - Fork 81
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
native: clear LastGasPerVote when voting for NULL #3349
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3349 +/- ##
==========================================
- Coverage 84.68% 84.67% -0.01%
==========================================
Files 331 331
Lines 44932 44935 +3
==========================================
Hits 38050 38050
- Misses 5371 5373 +2
- Partials 1511 1512 +1 ☔ View full report in Codecov by Sentry. |
86bea33
to
5456d9c
Compare
5456d9c
to
79da77d
Compare
399ec60
to
858f215
Compare
858f215
to
2b8254a
Compare
2b8254a
to
ac184c3
Compare
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.
Otherwise LGTM, the second test is not needed, since it'll require nontrivial modifications to cause error in the middle of voteInternal
, just doesn't worth it.
as := new(state.NEOBalance) | ||
err = as.FromStackItem(res) | ||
require.NoError(t, err) | ||
return as, res.Value().([]stackitem.Item)[3].Value().(*big.Int).Uint64() |
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.
Second return value is not needed, it's enough that you've checked require.Equal(t, 4, len(res.Value().([]stackitem.Item)))
.
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.
but the fourth one is zero integer
no need to check it?
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.
but the fourth one is zero integer no need to check it?
If you have 4 items, then (s *NEOBalance) FromStackItem
will deserialize 4-th item in some particular value. And in test you check that this value is 0. That's enough.
My primary concern here was about the check that 4-th item is present.
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.
neo-go/pkg/core/state/native_state.go
Lines 162 to 168 in 9c83bef
if len(structItem) >= 4 { | |
lastGasPerVote, err := structItem[3].TryInteger() | |
if err != nil { | |
return fmt.Errorf("invalid last vote reward per neo stackitem: %w", err) | |
} | |
s.LastGasPerVote = *lastGasPerVote | |
} |
Port neo-project/neo#3173. Close #3345 Signed-off-by: Ekaterina Pavlova <[email protected]>
ac184c3
to
0e6fbad
Compare
Port neo-project/neo#3173.
Close #3345