-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add From<FPDecimal> for Uint256
for FPDecimal
#217
base: dev
Are you sure you want to change the base?
Conversation
WalkthroughThe changes introduce a conversion implementation from Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant FPDecimal
participant Uint256
Client->>FPDecimal: Create FPDecimal instance
Client->>Uint256: Convert FPDecimal to Uint256 using From trait
Uint256-->>Client: Return Uint256 instance
Assessment against linked issues
Poem
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 (
|
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.
Hi @Aursen it'd be good if you added some test cases for this prior to merging. |
Yeah sure let me add 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.
impl From<FPDecimal> for Uint256 { | ||
fn from(value: FPDecimal) -> Self { | ||
value.to_u256() | ||
} |
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.
Add documentation for the From<FPDecimal> for Uint256
implementation.
It's important to document how the conversion handles the sign of FPDecimal
and any potential edge cases. This will help future developers understand the conversion process better and ensure correct usage.
#[test] | ||
fn test_into_u256() { | ||
let fp_decimal: Uint256 = FPDecimal { | ||
num: U256::from(12345u64), | ||
sign: 1, // Assuming it's always positive | ||
} | ||
.into(); | ||
let u256 = Uint256::from(12345u64); | ||
|
||
assert_eq!(u256, fp_decimal) | ||
} |
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.
Enhance the test coverage for test_into_u256
.
The current test only covers a basic positive number conversion. Consider adding tests for edge cases such as large numbers, negative numbers, and zero to ensure the robustness of the conversion process.
Closes #216
Summary by CodeRabbit
New Features
FPDecimal
toUint256
for improved compatibility.Tests
FPDecimal
toUint256
.