fix(api): XDPoS_getRewardByAccount to be able to parse scientific not#2301
fix(api): XDPoS_getRewardByAccount to be able to parse scientific not#2301wanwiset25 wants to merge 1 commit intodev-upgradefrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
wanwiset25
requested review from
anunay-xin,
benjamin202410,
gzliudan and
liam-lai
…ion in json file added unit tests formatting
wanwiset25
force-pushed
the
fix-reward-scientific-notation
branch
from
b651aa1 to
1b2b078
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes the XDPoS_getRewardByAccount reward parsing path to handle reward values encoded in JSON using scientific notation, and adds unit tests to validate the conversion behavior.
Changes:
- Added
jsonNumberToBigIntto convertjson.Numbervalues that may be in scientific notation into*big.Int. - Updated reward extraction logic to use the new conversion helper instead of
big.Int.SetString. - Added unit tests covering integer, scientific-notation, and invalid-number inputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| consensus/XDPoS/api.go | Adds jsonNumberToBigInt and wires it into reward parsing for masternode/protector/observer reward fields. |
| consensus/XDPoS/api_test.go | Adds unit tests for jsonNumberToBigInt and imports encoding/json. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Fall back to big.Float to handle scientific notation. | ||
| f, _, err := new(big.Float).SetPrec(256).Parse(s, 10) | ||
| if err != nil { | ||
| log.Warn("[jsonNumberToBigInt] Failed to parse json.Number:", "number", s, "err", err) | ||
| return nil, false |
There was a problem hiding this comment.
Using big.Float with a fixed 256-bit precision to parse JSON numbers can round large values; when this happens, converting to *big.Int may yield an incorrect integer. For reward amounts (which should be exact), consider an exact parsing strategy (e.g., mantissa+exponent to big.Int/big.Rat) instead of big.Float.
| // The value had a fractional part; truncate is the best we can do | ||
| log.Debug("[jsonNumberToBigInt] json.Number had fractional part, truncated to integer", "number", s, "truncated", i.String(), "accuracy", acc) |
There was a problem hiding this comment.
When acc != big.Exact, jsonNumberToBigInt currently logs and still returns (i, true). That means fractional inputs (or rounded big.Float parses) are silently truncated and treated as valid rewards. Consider returning ok=false (or error) when acc != big.Exact so callers don't accept corrupted/non-integer amounts.
| // The value had a fractional part; truncate is the best we can do | |
| log.Debug("[jsonNumberToBigInt] json.Number had fractional part, truncated to integer", "number", s, "truncated", i.String(), "accuracy", acc) | |
| log.Warn("[jsonNumberToBigInt] json.Number is not an exact integer value", "number", s, "truncated", i.String(), "accuracy", acc) | |
| return nil, false |
| got, ok := jsonNumberToBigInt(tt.input) | ||
| if tt.wantOk { | ||
| assert.True(t, ok, "input %q: parse failed, expected %s", tt.input, tt.want) | ||
| assert.Equal(t, 0, tt.want.Cmp(got), "input %q: expected %s but got %s", tt.input, tt.want, got) | ||
| } else { |
There was a problem hiding this comment.
In the wantOk branch, assert.True(t, ok, ...) doesn't stop the test on failure, so the next line can call tt.want.Cmp(got) with got=nil and panic. Use require.True (or guard with if !ok { return }) before comparing.
| assert.False(t, ok, "input %q: expected parse failure but got %s", tt.input, got) | ||
| assert.Nil(t, got, "input %q: expected nil but got %s", tt.input, got) |
There was a problem hiding this comment.
The failure-branch assertion messages format got with %s (e.g., "... got %s") even though got can be nil; this can lead to confusing output or a nil formatting panic depending on the formatter path. Prefer %v (or print got.String() only after a nil check).
| assert.False(t, ok, "input %q: expected parse failure but got %s", tt.input, got) | |
| assert.Nil(t, got, "input %q: expected nil but got %s", tt.input, got) | |
| assert.False(t, ok, "input %q: expected parse failure but got %v", tt.input, got) | |
| assert.Nil(t, got, "input %q: expected nil but got %v", tt.input, got) |
Proposed changes
fix api XDPoS_getRewardByAccount to be able to parse scientific not
added unit tests
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that