epic: nibiru-std::math::{ SdkDec, DecimalExt } #106
Conversation
WalkthroughThe changes reflect updates to Wasm file checksums, enhancements to error handling, and revisions to contract and standard library code. New mathematical types and methods have been introduced, along with changes to import statements and enum variants. These updates aim to improve the robustness and functionality of the smart contracts and underlying libraries. Changes
Assessment against linked issues
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 X ? TipsChat with CodeRabbit Bot (
|
Unique-Divine
added
type: enhancement
Codecov Report
Additional details and impacted files
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: CodeRabbit UI
Files ignored due to filter (7)
- Cargo.lock
- Cargo.toml
- artifacts/nibi_stargate.wasm
- artifacts/nibi_stargate_perp.wasm
- artifacts/nusd_valuator.wasm
- artifacts/shifter.wasm
- nibiru-std/Cargo.toml
Files selected for processing (11)
- artifacts/checksums.txt (1 hunks)
- artifacts/checksums_intermediate.txt (1 hunks)
- contracts/core-shifter/README.md (1 hunks)
- contracts/core-shifter/src/contract.rs (2 hunks)
- contracts/core-shifter/src/error.rs (2 hunks)
- contracts/core-shifter/src/msgs.rs (2 hunks)
- nibiru-std/src/errors.rs (1 hunks)
- nibiru-std/src/lib.rs (1 hunks)
- nibiru-std/src/math.rs (1 hunks)
- nibiru-std/src/proto/buf/nibiru.oracle.v1.rs (14 hunks)
- nibiru-std/src/proto/mod.rs (1 hunks)
Files skipped from review due to trivial changes (4)
- artifacts/checksums_intermediate.txt
- nibiru-std/src/lib.rs
- nibiru-std/src/proto/buf/nibiru.oracle.v1.rs
- nibiru-std/src/proto/mod.rs
Additional comments: 18
artifacts/checksums.txt (1)
- 5-9: Checksum updates for Wasm files are consistent with the PR's objectives to update Wasm bytecode artifacts. Ensure that the updated checksums correspond to the newly compiled Wasm files and that they have been tested to confirm they work as expected.
contracts/core-shifter/README.md (3)
24-36: The addition of
shifter-0-inst.jsonand corresponding bash command for contract initialization is clear and provides a straightforward example for users to follow. Ensure that the address provided in the JSON snippet is a placeholder or example and not a real address, to avoid any security or privacy concerns.51-65: The
sudo-add.jsonconfiguration and the correspondingnibidcommand for editing the sudo are clear. However, as with the initialization JSON, ensure that the addresses provided are placeholders or examples and not real addresses.71-82: The
shifter-1-exec-shift-peg.jsonconfiguration and the corresponding execution command are clear and provide a good example of how to use the contract's functionality. Again, ensure that the pair and new peg multiplier values are generic examples.contracts/core-shifter/src/contract.rs (2)
73-79: The conversion of
new_peg_multtoSdkDectype usingSdkDec::from_str(&new_peg_mult)?.pb_repr()is a significant change. Ensure that thepb_reprmethod correctly convertsSdkDecfor protobuf representation and that the error handling for potential parsing failures is adequate.1-14: The addition of
use std::str::FromStr;and the changes to the import statements innibiru_stdare in line with the PR's objectives to implement new types and extensions for decimal arithmetic.contracts/core-shifter/src/error.rs (3)
3-4: The import statement for
nibiru_std::errorsis correctly added to support the newMathErrorvariant in theContractErrorenum.17-18: The addition of the
MathErrorvariant to theContractErrorenum is consistent with the PR objectives and the AI-generated summary. The#[from]attribute is correctly used for error conversion.13-19: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-19]
Ensure that all math-related errors in the
core-shiftercontract are updated to use the newMathErrorvariant for consistency and accurate error handling.contracts/core-shifter/src/msgs.rs (2)
1-5: The import of
Uint256suggests that the contract is now using this type for arithmetic operations, which aligns with the PR's objective to enhance decimal arithmetic capabilities.19-25: The change of
new_peg_multfromDecimaltoStringis consistent with the PR's goal of improving protobuf representations and using the newSdkDectype. Ensure that the corresponding deserialization and validation logic is updated to handle theStringtype and convert it toSdkDecas needed.Verification successful
The
from_strmethod forSdkDecis implemented innibiru-std/src/math.rsand returns aResult, indicating that it includes error handling which serves as validation when converting from aString. This aligns with the changes in thecore-shiftercontract and supports the PR's goal of using theSdkDectype for improved protobuf representations. The verification process has not revealed any issues with the deserialization and validation logic fornew_peg_mult.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for deserialization logic related to `new_peg_mult` in the `core-shifter` contract. rg 'new_peg_mult' --type rust --glob 'contracts/core-shifter/src/*' # Verify that validation logic for `new_peg_mult` as a string is present and correct. rg 'validate_.*new_peg_mult' --type rust --glob 'contracts/core-shifter/src/*'Length of output: 432
Script:
#!/bin/bash # Search for the `from_str` method implementation for `SdkDec`. rg 'impl SdkDec' --type rust --glob 'nibiru-std/src/*' rg 'fn from_str' --type rust --glob 'nibiru-std/src/*'Length of output: 304
nibiru-std/src/math.rs (7)
14-21: The
Signenum is well-defined with appropriate variants for representing the sign of a number.24-30: The
DecimalExtstruct is introduced with fields for sign and decimal value, which seems correct for the intended functionality.98-108: The
quomethod correctly handles division and checks for division by zero, returning aMathErrorif the divisor is zero.124-156: The
FromStrimplementation forDecimalExtcorrectly parses a decimal string, handling negative values and ensuring the parsed value has the correct sign.166-191: The
SdkDecstruct and its associated methods are introduced for representing protobuf strings. The methods seem to correctly handle the conversion fromDecimalExt.226-287: The method
to_sdk_dec_pb_reprinDecimalExtcorrectly encodes the decimal to a protobuf representation. However, there is a potential issue with the handling of negative numbers. The method checks for a negative sign and inserts it if necessary, but it does not account for the possibility of thecw::Decimalalready being negative, which could lead to double negation.- if neg { - sdk_dec.insert(0, '-'); - }This needs to be verified to ensure that
cw::Decimaldoes not already include the sign in its string representation.
- 290-338: The method
from_sdk_decinDecimalExtcorrectly decodes a protobuf representation string back into aDecimalExt. It handles both integer-only and fractional cases and checks for invalid formats.
| # FOR anyone | ||
| alias tx="jq -rcs '.[0].txhash' | { read txhash; sleep 3; nibid q tx \$txhash | jq '{txhash, height, code, logs, tx, gas_wanted, gas_used}' >> out.json}" |
There was a problem hiding this comment.
The alias tx is defined for extracting the transaction hash and querying transaction details. It's a useful addition for users to quickly retrieve transaction information. However, consider adding a comment explaining the purpose of each part of the command for users who might not be familiar with jq or the other command-line tools used here.
| ``` | ||
|
|
||
| ```bash | ||
| nibid tx wasm inst 1 "$(cat shifter-0-inst.json)" --label="Crazy tester" --no-admin --from=$KEY_NAME -y | tx |
There was a problem hiding this comment.
The command for contract instantiation using nibid is provided, but it might be helpful to include a brief explanation of the parameters used in the command, such as --label, --no-admin, and --from=$KEY_NAME, to aid users in understanding their purpose and how to modify them if necessary.
|
|
||
| ```bash | ||
| # FOR ME | ||
| alias tx="jq -rcs '.[0].txhash' | { read txhash; sleep 3; nibid q tx \$txhash | jq '{txhash, height, code, logs, gas_used, gas_wanted, tx}' | vv}" |
There was a problem hiding this comment.
The alias tx is redefined here with a different output command (vv). This could be confusing for users, as the same alias is used for different purposes. It would be better to use distinct alias names for different operations to avoid any ambiguity.
| CONTRACT=nibi14hj2tavq8fpesdwxxcu44rty3hh90vhujrvcmstl4zr3txmfvw9ssa9gcs | ||
| CONTRACT=nibi1yyca08xqdgvjz0psg56z67ejh9xms6l436u8y58m82npdqqhmmtqzvacjr |
There was a problem hiding this comment.
Duplicate CONTRACT environment variable declarations are present. This seems like an oversight and could lead to confusion. Only one of these lines should be kept, preferably the one that is intended to be used in subsequent commands.
| nibid | ||
| ``` |
There was a problem hiding this comment.
An incomplete bash command is present. This should either be completed with the correct command or removed if it was added by mistake.
| pub fn add(&self, other: Self) -> Self { | ||
| if self.sign == other.sign { | ||
| return DecimalExt { | ||
| sign: self.sign, | ||
| dec: self.dec.add(other.dec), | ||
| }; | ||
| } else if other.dec.is_zero() { | ||
| } | ||
|
|
||
| let self_dec_gt: bool = self.dec.ge(&other.dec); | ||
| let sign = if self_dec_gt { self.sign } else { other.sign }; | ||
| let dec = if self_dec_gt { | ||
| self.dec.sub(other.dec) // if abs(self.dec) > abs(other.dec) | ||
| } else { | ||
| other.dec.sub(self.dec) // if abs(self.dec) < abs(other.dec) | ||
| }; | ||
| let sign = if dec.is_zero() { Sign::Zero } else { sign }; | ||
|
|
||
| DecimalExt { sign, dec } |
There was a problem hiding this comment.
The add method in DecimalExt correctly handles the addition of two decimals considering their signs. However, there is an empty else block at lines 53-54 which should be removed to avoid confusion.
- } else if other.dec.is_zero() {
- }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.
| pub fn add(&self, other: Self) -> Self { | |
| if self.sign == other.sign { | |
| return DecimalExt { | |
| sign: self.sign, | |
| dec: self.dec.add(other.dec), | |
| }; | |
| } else if other.dec.is_zero() { | |
| } | |
| let self_dec_gt: bool = self.dec.ge(&other.dec); | |
| let sign = if self_dec_gt { self.sign } else { other.sign }; | |
| let dec = if self_dec_gt { | |
| self.dec.sub(other.dec) // if abs(self.dec) > abs(other.dec) | |
| } else { | |
| other.dec.sub(self.dec) // if abs(self.dec) < abs(other.dec) | |
| }; | |
| let sign = if dec.is_zero() { Sign::Zero } else { sign }; | |
| DecimalExt { sign, dec } | |
| pub fn add(&self, other: Self) -> Self { | |
| if self.sign == other.sign { | |
| return DecimalExt { | |
| sign: self.sign, | |
| dec: self.dec.add(other.dec), | |
| }; | |
| } | |
| let self_dec_gt: bool = self.dec.ge(&other.dec); | |
| let sign = if self_dec_gt { self.sign } else { other.sign }; | |
| let dec = if self_dec_gt { | |
| self.dec.sub(other.dec) // if abs(self.dec) > abs(other.dec) | |
| } else { | |
| other.dec.sub(self.dec) // if abs(self.dec) < abs(other.dec) | |
| }; | |
| let sign = if dec.is_zero() { Sign::Zero } else { sign }; | |
| DecimalExt { sign, dec } |
| assert_eq!(neg_num, pos_num.neg()); | ||
| assert_eq!(pos_num, neg_num.neg()); | ||
| assert_eq!(zero_num, zero_num.neg()); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn mul() -> TestResult { | ||
| let test_cases: &[(&str, &str, &str)] = &[ | ||
| ("0", "0", "0"), | ||
| ("0", "420", "0"), | ||
| ("16", "16", "256"), | ||
| ("5", "-3", "-15"), | ||
| ("-7", "7", "-49"), | ||
| ]; | ||
| for &(a, b, want_product) in test_cases.iter() { | ||
| let a = DecimalExt::from_str(a)?; | ||
| let b = DecimalExt::from_str(b)?; | ||
| let want_product = DecimalExt::from_str(want_product)?; | ||
| let got_product = a.mul(b); | ||
| assert_eq!(want_product, got_product); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn quo() -> TestResult { | ||
| let test_cases: &[(&str, &str, &str)] = &[ | ||
| ("0", "420", "0"), | ||
| ("256", "16", "16"), | ||
| ("-15", "5", "-3"), | ||
| ("-49", "-7", "7"), | ||
| ]; | ||
| for &(a, b, want_quo) in test_cases.iter() { | ||
| let a = DecimalExt::from_str(a)?; | ||
| let b = DecimalExt::from_str(b)?; | ||
| let want_quo = DecimalExt::from_str(want_quo)?; | ||
| let got_quo = a.quo(b)?; | ||
| assert_eq!(want_quo, got_quo); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn sdk_dec_int_only() -> TestResult { | ||
| let test_cases: &[(&str, &str)] = &[ | ||
| // Zero cases should all be equal | ||
| ("0", &"0".repeat(18)), | ||
| ("000.00", &"0".repeat(18)), | ||
| ("0.00", &"0".repeat(18)), | ||
| ("00000", &"0".repeat(18)), | ||
| // Non-zero cases | ||
| ("10", &format!("10{}", "0".repeat(18))), | ||
| ("-10", &format!("-10{}", "0".repeat(18))), | ||
| ("123", &format!("123{}", "0".repeat(18))), | ||
| ("-123", &format!("-123{}", "0".repeat(18))), | ||
| ]; | ||
|
|
||
| for tc in test_cases.iter() { | ||
| let (arg, want_sdk_dec) = tc; | ||
| let want_dec: DecimalExt = DecimalExt::from_str(arg)?; | ||
| let got_sdk_dec: String = want_dec.to_sdk_dec_pb_repr()?; | ||
| assert_eq!(want_sdk_dec.to_owned(), got_sdk_dec); | ||
|
|
||
| let got_dec = DecimalExt::from_sdk_dec(&got_sdk_dec)?; | ||
| assert_eq!(want_dec, got_dec) | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// to_sdk_dec test with fractional parts | ||
| #[test] | ||
| fn sdk_dec_fractional() -> TestResult { | ||
| let test_cases: &[(&str, &str)] = &[ | ||
| ("0.5", &format!("05{}", "0".repeat(17))), | ||
| ("0.005", &format!("0005{}", "0".repeat(15))), | ||
| ("123.456", &format!("123456{}", "0".repeat(15))), | ||
| ("-123.456", &format!("-123456{}", "0".repeat(15))), | ||
| ("0.00596", &format!("000596{}", "0".repeat(13))), | ||
| ("13.5", &format!("135{}", "0".repeat(17))), | ||
| ("-13.5", &format!("-135{}", "0".repeat(17))), | ||
| ("1574.00005", &format!("157400005{}", "0".repeat(13))), | ||
| ]; | ||
|
|
||
| for tc in test_cases.iter() { | ||
| let (arg, want_sdk_dec) = tc; | ||
| let want_dec: DecimalExt = DecimalExt::from_str(arg)?; | ||
| let got_sdk_dec: String = want_dec.to_sdk_dec_pb_repr()?; | ||
| assert_eq!(want_sdk_dec.to_owned(), got_sdk_dec); | ||
|
|
||
| let got_dec = DecimalExt::from_sdk_dec(&got_sdk_dec)?; | ||
| assert_eq!(want_dec, got_dec) | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn blank_test() -> TestResult { | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The test cases provided in the #[cfg(test)] module cover various scenarios for the new methods and types. However, there is a blank test at lines 518-520 that should either be implemented or removed.
- #[test]
- fn blank_test() -> TestResult {
- Ok(())
- }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.
| #[cfg(test)] | |
| mod test_sign_dec { | |
| use cosmwasm_std as cw; | |
| use std::str::FromStr; | |
| use crate::{ | |
| errors::TestResult, | |
| math::{DecimalExt, Sign}, | |
| }; | |
| #[test] | |
| fn default_is_zero() -> TestResult { | |
| assert_eq!( | |
| DecimalExt::default(), | |
| DecimalExt { | |
| sign: Sign::Zero, | |
| dec: cw::Decimal::from_str("0")? | |
| } | |
| ); | |
| assert_eq!(DecimalExt::default(), DecimalExt::zero()); | |
| assert_eq!(DecimalExt::zero(), cw::Decimal::from_str("0")?.into()); | |
| Ok(()) | |
| } | |
| #[test] | |
| fn from_cw() -> TestResult { | |
| assert_eq!( | |
| DecimalExt::default(), | |
| DecimalExt::from(cw::Decimal::from_str("0")?) | |
| ); | |
| let cw_dec = cw::Decimal::from_str("123.456")?; | |
| assert_eq!( | |
| DecimalExt { | |
| sign: Sign::Positive, | |
| dec: cw_dec | |
| }, | |
| DecimalExt::from(cw_dec) | |
| ); | |
| let num = "123.456"; | |
| assert_eq!( | |
| DecimalExt { | |
| sign: Sign::Negative, | |
| dec: cw::Decimal::from_str(num)? | |
| }, | |
| DecimalExt::from_str(&format!("-{}", num))? | |
| ); | |
| Ok(()) | |
| } | |
| // TODO: How will you handle overflow? | |
| #[test] | |
| fn add() -> TestResult { | |
| let test_cases: &[(&str, &str, &str)] = &[ | |
| ("0", "0", "0"), | |
| ("0", "420", "420"), | |
| ("69", "420", "489"), | |
| ("5", "-3", "2"), | |
| ("-7", "7", "0"), | |
| ("-420", "69", "-351"), | |
| ("-69", "420", "351"), | |
| ]; | |
| for &(a, b, want_sum_of) in test_cases.iter() { | |
| let a = DecimalExt::from_str(a)?; | |
| let b = DecimalExt::from_str(b)?; | |
| let want_sum_of = DecimalExt::from_str(want_sum_of)?; | |
| let got_sum_of = a.add(b); | |
| assert_eq!(want_sum_of, got_sum_of); | |
| } | |
| Ok(()) | |
| } | |
| #[test] | |
| fn neg() -> TestResult { | |
| let pos_num = DecimalExt::from_str("69")?; | |
| let neg_num = DecimalExt::from_str("-69")?; | |
| let zero_num = DecimalExt::zero(); | |
| assert_eq!(neg_num, pos_num.neg()); | |
| assert_eq!(pos_num, neg_num.neg()); | |
| assert_eq!(zero_num, zero_num.neg()); | |
| Ok(()) | |
| } | |
| #[test] | |
| fn mul() -> TestResult { | |
| let test_cases: &[(&str, &str, &str)] = &[ | |
| ("0", "0", "0"), | |
| ("0", "420", "0"), | |
| ("16", "16", "256"), | |
| ("5", "-3", "-15"), | |
| ("-7", "7", "-49"), | |
| ]; | |
| for &(a, b, want_product) in test_cases.iter() { | |
| let a = DecimalExt::from_str(a)?; | |
| let b = DecimalExt::from_str(b)?; | |
| let want_product = DecimalExt::from_str(want_product)?; | |
| let got_product = a.mul(b); | |
| assert_eq!(want_product, got_product); | |
| } | |
| Ok(()) | |
| } | |
| #[test] | |
| fn quo() -> TestResult { | |
| let test_cases: &[(&str, &str, &str)] = &[ | |
| ("0", "420", "0"), | |
| ("256", "16", "16"), | |
| ("-15", "5", "-3"), | |
| ("-49", "-7", "7"), | |
| ]; | |
| for &(a, b, want_quo) in test_cases.iter() { | |
| let a = DecimalExt::from_str(a)?; | |
| let b = DecimalExt::from_str(b)?; | |
| let want_quo = DecimalExt::from_str(want_quo)?; | |
| let got_quo = a.quo(b)?; | |
| assert_eq!(want_quo, got_quo); | |
| } | |
| Ok(()) | |
| } | |
| #[test] | |
| fn sdk_dec_int_only() -> TestResult { | |
| let test_cases: &[(&str, &str)] = &[ | |
| // Zero cases should all be equal | |
| ("0", &"0".repeat(18)), | |
| ("000.00", &"0".repeat(18)), | |
| ("0.00", &"0".repeat(18)), | |
| ("00000", &"0".repeat(18)), | |
| // Non-zero cases | |
| ("10", &format!("10{}", "0".repeat(18))), | |
| ("-10", &format!("-10{}", "0".repeat(18))), | |
| ("123", &format!("123{}", "0".repeat(18))), | |
| ("-123", &format!("-123{}", "0".repeat(18))), | |
| ]; | |
| for tc in test_cases.iter() { | |
| let (arg, want_sdk_dec) = tc; | |
| let want_dec: DecimalExt = DecimalExt::from_str(arg)?; | |
| let got_sdk_dec: String = want_dec.to_sdk_dec_pb_repr()?; | |
| assert_eq!(want_sdk_dec.to_owned(), got_sdk_dec); | |
| let got_dec = DecimalExt::from_sdk_dec(&got_sdk_dec)?; | |
| assert_eq!(want_dec, got_dec) | |
| } | |
| Ok(()) | |
| } | |
| /// to_sdk_dec test with fractional parts | |
| #[test] | |
| fn sdk_dec_fractional() -> TestResult { | |
| let test_cases: &[(&str, &str)] = &[ | |
| ("0.5", &format!("05{}", "0".repeat(17))), | |
| ("0.005", &format!("0005{}", "0".repeat(15))), | |
| ("123.456", &format!("123456{}", "0".repeat(15))), | |
| ("-123.456", &format!("-123456{}", "0".repeat(15))), | |
| ("0.00596", &format!("000596{}", "0".repeat(13))), | |
| ("13.5", &format!("135{}", "0".repeat(17))), | |
| ("-13.5", &format!("-135{}", "0".repeat(17))), | |
| ("1574.00005", &format!("157400005{}", "0".repeat(13))), | |
| ]; | |
| for tc in test_cases.iter() { | |
| let (arg, want_sdk_dec) = tc; | |
| let want_dec: DecimalExt = DecimalExt::from_str(arg)?; | |
| let got_sdk_dec: String = want_dec.to_sdk_dec_pb_repr()?; | |
| assert_eq!(want_sdk_dec.to_owned(), got_sdk_dec); | |
| let got_dec = DecimalExt::from_sdk_dec(&got_sdk_dec)?; | |
| assert_eq!(want_dec, got_dec) | |
| } | |
| Ok(()) | |
| } | |
| #[test] | |
| fn blank_test() -> TestResult { | |
| Ok(()) | |
| } |
Unique-Divine
force-pushed
the
realu/e2e-init
branch
from
444f243 to
d29eee2
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (7)
- Cargo.lock
- Cargo.toml
- artifacts/nibi_stargate.wasm
- artifacts/nibi_stargate_perp.wasm
- artifacts/nusd_valuator.wasm
- artifacts/shifter.wasm
- nibiru-std/Cargo.toml
Files selected for processing (12)
- .github/codecov.yml (1 hunks)
- artifacts/checksums.txt (1 hunks)
- artifacts/checksums_intermediate.txt (1 hunks)
- contracts/core-shifter/README.md (1 hunks)
- contracts/core-shifter/src/contract.rs (2 hunks)
- contracts/core-shifter/src/error.rs (2 hunks)
- contracts/core-shifter/src/msgs.rs (2 hunks)
- nibiru-std/src/errors.rs (1 hunks)
- nibiru-std/src/lib.rs (1 hunks)
- nibiru-std/src/math.rs (1 hunks)
- nibiru-std/src/proto/buf/nibiru.oracle.v1.rs (14 hunks)
- nibiru-std/src/proto/mod.rs (1 hunks)
Files skipped from review due to trivial changes (6)
- .github/codecov.yml
- artifacts/checksums.txt
- artifacts/checksums_intermediate.txt
- nibiru-std/src/lib.rs
- nibiru-std/src/proto/buf/nibiru.oracle.v1.rs
- nibiru-std/src/proto/mod.rs
Files skipped from review as they are similar to previous changes (4)
- contracts/core-shifter/README.md
- contracts/core-shifter/src/contract.rs
- contracts/core-shifter/src/error.rs
- contracts/core-shifter/src/msgs.rs
Additional comments: 5
nibiru-std/src/math.rs (5)
14-21: The
Signenum is well-defined and uses the#[default]attribute to setZeroas the default value, which is a good practice for clarity and safety.98-101: The error handling for division by zero in the
quomethod is correctly implemented, ensuring robust arithmetic operations.127-154: The
FromStrimplementation forDecimalExtis comprehensive, covering various input cases and error handling, which aligns with the PR's objectives to improve decimal representations.166-191: The
SdkDecstruct and associated methods provide a clear way to handle protobuf representations of decimals, which is part of the PR's objectives to improve protobuf representations.226-287: The methods
to_sdk_dec_pb_reprandfrom_sdk_decinDecimalExtare implemented with thorough error handling and validation, ensuring that the protobuf representations are correctly managed.
| #[error("{0}")] | ||
| MathError(#[from] MathError), | ||
| } | ||
|
|
||
| #[derive(Error, Debug, PartialEq)] | ||
| pub enum MathError { | ||
| #[error("division by zero not well defined")] | ||
| DivisionByZero, | ||
|
|
||
| #[error("could not parse decimal from string \"{}\": {}", dec_str, err)] | ||
| CwDecParseError { dec_str: String, err: cw::StdError }, | ||
|
|
||
| #[error("could not parse to cosmosdk.io/math.LegacyDec: {0}")] | ||
| SdkDecError(String), |
There was a problem hiding this comment.
The addition of new error variants to MathError and the inclusion of MathError in NibiruError are correctly implemented. However, consider providing more context for the SdkDecError variant to aid in debugging, similar to the detailed message in CwDecParseError.
| pub fn add(&self, other: Self) -> Self { | ||
| if self.sign == other.sign { | ||
| return DecimalExt { | ||
| sign: self.sign, | ||
| dec: self.dec.add(other.dec), | ||
| }; | ||
| } else if other.dec.is_zero() { | ||
| } |
There was a problem hiding this comment.
The empty else if block at lines 53-54 should be removed as it serves no purpose and could lead to confusion.
- } else if other.dec.is_zero() {
- }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.
| pub fn add(&self, other: Self) -> Self { | |
| if self.sign == other.sign { | |
| return DecimalExt { | |
| sign: self.sign, | |
| dec: self.dec.add(other.dec), | |
| }; | |
| } else if other.dec.is_zero() { | |
| } | |
| pub fn add(&self, other: Self) -> Self { | |
| if self.sign == other.sign { | |
| return DecimalExt { | |
| sign: self.sign, | |
| dec: self.dec.add(other.dec), | |
| }; |
| #[test] | ||
| fn blank_test() -> TestResult { | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The blank test at lines 518-520 should either be implemented or removed to avoid confusion and maintain clean code.
- #[test]
- fn blank_test() -> TestResult {
- Ok(())
- }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.
| #[test] | |
| fn blank_test() -> TestResult { | |
| Ok(()) | |
| } |
nibiru.perp.v2.MsgShift*#104Summary by CodeRabbit
New Features
Documentation
Refactor
Style
Chores
Bug Fixes