Add uint256 package and int256 wrapper #1320
Conversation
WalkthroughThe recent update introduces a signed 256-bit integer type, Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
protocol/go.modis excluded by!**/*.mod,!**/*.modprotocol/go.sumis excluded by!**/*.sum,!**/*.sum
Files selected for processing (2)
- protocol/lib/int256/int256.go (1 hunks)
- protocol/lib/int256/int256_test.go (1 hunks)
Additional Context Used
Path-based Instructions (2)
protocol/lib/int256/int256.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/lib/int256/int256_test.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments not posted (4)
protocol/lib/int256/int256.go (3)
11-12: Consider adding thread-safety documentation or mechanisms, especially given the warning about pointer usage.
32-37: Ensure that the conversion fromint64touint64for negative values is intended and correctly handles potential overflow.
167-185: Verify the correctness ofmulExp10function, especially the handling of negative exponents and the fallback toExpwhen the exponent is not in the lookup.protocol/lib/int256/int256_test.go (1)
34-57: Refactor the test forSetto include assertions that verify the original value ofxis not modified by the operation.
| // Set sets z to the value of a uint256. | ||
| func (z *Int) SetUint64(x uint64) *Int { | ||
| return (*Int)((*uint256.Int)(z).SetUint64(x)) | ||
| } |
There was a problem hiding this comment.
The comment above this method incorrectly states "Set sets z to the value of a uint256." It should be corrected to reflect the actual functionality.
- // Set sets z to the value of a uint256.
+ // SetUint64 sets z to the value of a uint64.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // Set sets z to the value of a uint256. | |
| func (z *Int) SetUint64(x uint64) *Int { | |
| return (*Int)((*uint256.Int)(z).SetUint64(x)) | |
| } | |
| // SetUint64 sets z to the value of a uint64. | |
| func (z *Int) SetUint64(x uint64) *Int { | |
| return (*Int)((*uint256.Int)(z).SetUint64(x)) | |
| } |
| // Div sets z = x / y and returns z. If y is 0, z is set to 0. | ||
| func (z *Int) Div(x, y *Int) *Int { | ||
| return (*Int)((*uint256.Int)(z).SDiv((*uint256.Int)(x), (*uint256.Int)(y))) |
There was a problem hiding this comment.
Add a check for division by zero to prevent potential runtime panics.
+ if y.IsZero() {
+ return z.Set(NewInt(0))
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // Div sets z = x / y and returns z. If y is 0, z is set to 0. | |
| func (z *Int) Div(x, y *Int) *Int { | |
| return (*Int)((*uint256.Int)(z).SDiv((*uint256.Int)(x), (*uint256.Int)(y))) | |
| // Div sets z = x / y and returns z. If y is 0, z is set to 0. | |
| func (z *Int) Div(x, y *Int) *Int { | |
| if y.IsZero() { | |
| return z.Set(NewInt(0)) | |
| } | |
| return (*Int)((*uint256.Int)(z).SDiv((*uint256.Int)(x), (*uint256.Int)(y))) |
| func createExp10Lookup() map[uint64]uint256.Int { | ||
| lookup := make(map[uint64]uint256.Int, 100) | ||
| value := uint256.NewInt(1) | ||
| for i := 0; i < 100; i++ { | ||
| lookup[uint64(i)] = *new(uint256.Int).Set(value) | ||
| value.Mul(value, (*uint256.Int)(TenInt256)) | ||
| } | ||
| return lookup |
There was a problem hiding this comment.
Consider preallocating the exp10Lookup map with a size of 19 instead of 100, as the maximum exponent for base 10 in a uint256 cannot exceed 18.
- lookup := make(map[uint64]uint256.Int, 100)
+ lookup := make(map[uint64]uint256.Int, 19)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func createExp10Lookup() map[uint64]uint256.Int { | |
| lookup := make(map[uint64]uint256.Int, 100) | |
| value := uint256.NewInt(1) | |
| for i := 0; i < 100; i++ { | |
| lookup[uint64(i)] = *new(uint256.Int).Set(value) | |
| value.Mul(value, (*uint256.Int)(TenInt256)) | |
| } | |
| return lookup | |
| func createExp10Lookup() map[uint64]uint256.Int { | |
| lookup := make(map[uint64]uint256.Int, 19) | |
| value := uint256.NewInt(1) | |
| for i := 0; i < 100; i++ { | |
| lookup[uint64(i)] = *new(uint256.Int).Set(value) | |
| value.Mul(value, (*uint256.Int)(TenInt256)) | |
| } | |
| return lookup |
There was a problem hiding this comment.
Okay fair, but you're confusing it with a uint64. you can fit 10^79 in a uint256 and 10^78 in an int256.
There was a problem hiding this comment.
@roy-dydx, thank you for pointing out the confusion regarding the capacity of uint256 and int256. You're absolutely right; uint256 can indeed hold values up to 10^79, and int256 up to 10^78, which significantly exceeds the limits of a uint64. My initial suggestion to preallocate the exp10Lookup map with a size of 19 was based on an incorrect assumption about the maximum exponent for base 10 that these types can handle. Given this correction, it's clear that the map size should be reconsidered to accommodate the actual range of values that uint256 and int256 can represent. I appreciate your feedback and the opportunity to correct this misunderstanding.
🐰
New Learnings Added
User: roy-dydx"
URL: https://github.com/dydxprotocol/v4-chain/pull/1320
Timestamp: 2024-04-03T03:59:36.096Z
Learning: The `uint256` type can hold values up to 10^79, and `int256` can hold values up to 10^78, significantly exceeding the limits of a `uint64`.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
Changelist
Test Plan
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.