fix: converted seconds to milliseconds #578
Conversation
WalkthroughThe pull request updates the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
cowboy0015
requested review from
crnbarr93 and
joemonem
and removed request for
crnbarr93
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
packages/andromeda-finance/src/vesting.rs (3)
36-44: LGTM: Time units converted to Milliseconds, minor comment update neededThe changes from
u64toMillisecondsin theExecuteMsgenum are consistent with the PR objective of converting time units to milliseconds. This includes:
up_to_timeinClaimAlllockup_durationandrelease_unitinCreateBatchThese changes improve type safety and align with the standard practice in other ADOs.
Consider updating the comment on line 43 from "seconds" to "milliseconds" for consistency:
- /// How often releases occur in seconds. + /// How often releases occur in milliseconds.
92-99: LGTM: Time units converted to Milliseconds in BatchResponse, minor comment update neededThe changes from
u64toMillisecondsin theBatchResponsestruct are consistent with the PR objective:
lockup_endrelease_unitlast_claimed_release_timeThese changes improve type safety and align with the standard practice in other ADOs.
Consider updating the comment on line 98 from "seconds" to "milliseconds" for consistency:
- /// The time at which the last claim took place in seconds. + /// The time at which the last claim took place in milliseconds.
Incomplete Replacement of Time-related
u64InstancesThe verification script identified instances where
u64is still used in time-related contexts alongsideMilliseconds. Specifically:
contracts/fungible-tokens/andromeda-lockdrop/src/contract.rs: 50u64 * time_left.milliseconds()These remaining
u64usages should be converted toMillisecondsto ensure consistency and prevent potential bugs.🔗 Analysis chain
Line range hint
1-99: Summary: Successful conversion of time units to millisecondsThe changes in this file successfully implement the conversion from seconds to milliseconds as per the PR objectives:
- All time-related fields previously using
u64have been updated to use theMillisecondstype.- The changes are consistent across the
ExecuteMsgenum andBatchResponsestruct.- The import statement has been updated to include
Millisecondsfrom thecommonmodule.These modifications enhance type safety and align the contract's time measurement with the standard practice observed in other ADOs. The changes address the issue raised in SUPP-116 and contribute to maintaining consistency across the codebase.
To ensure that all instances of
u64related to time measurements have been replaced, run the following verification script:This script will help identify any remaining
u64instances that might be time-related and confirm the usage ofMillisecondsfor time-related fields.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all time-related u64 instances have been replaced with Milliseconds # Test: Search for any remaining u64 usages that might be time-related rg --type rust 'u64.*(?i)(time|duration|period|interval|frequency)' # Test: Confirm Milliseconds is used for time-related fields rg --type rust 'Milliseconds.*(?i)(time|duration|period|interval|frequency)'Length of output: 25163
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
- CHANGELOG.md (1 hunks)
- contracts/finance/andromeda-vesting/Cargo.toml (1 hunks)
- contracts/finance/andromeda-vesting/src/contract.rs (11 hunks)
- contracts/finance/andromeda-vesting/src/state.rs (5 hunks)
- contracts/finance/andromeda-vesting/src/testing/tests.rs (53 hunks)
- packages/andromeda-finance/src/vesting.rs (3 hunks)
✅ Files skipped from review due to trivial changes (1)
- contracts/finance/andromeda-vesting/Cargo.toml
🧰 Additional context used
🔇 Additional comments (45)
packages/andromeda-finance/src/vesting.rs (1)
4-4: LGTM: Import statement updated correctlyThe addition of
Millisecondsto the import statement from thecommonmodule is consistent with its usage throughout the file and aligns with the PR objective of converting time units to milliseconds.CHANGELOG.md (2)
53-53: LGTM! The change aligns with the PR objectives.The changelog entry correctly reflects the change from seconds to milliseconds in the Vesting ADO, which aligns with the PR objectives and the linked issue SUPP-116. This change promotes consistency across ADOs in time measurement.
Line range hint
1-53: Verify consistency and completeness of the changelog.While the change to milliseconds in the Vesting ADO is correctly documented, it's important to ensure that all related changes are captured in the changelog.
Please run the following script to check for any mentions of time units in the changed files:
This will help ensure that all relevant changes related to the time unit modification are properly documented in the changelog.
✅ Verification successful
The changelog is consistent and complete with respect to the time unit changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for mentions of time units in changed files # Test: Search for mentions of seconds, milliseconds, or time units in changed files git diff --name-only HEAD~1 | xargs rg -i '(second|millisecond|time unit)' # Test: Check if there are any other changes in the vesting-related files that might need to be mentioned in the changelog git diff --name-only HEAD~1 | grep -i 'vesting' | xargs rg -i '(change|update|modify)'Length of output: 755
contracts/finance/andromeda-vesting/src/state.rs (6)
2-3: ImportMillisecondsType CorrectlyThe
Millisecondstype is imported properly fromandromeda_std::common, aligning with the updated time representation in the contract.
23-23: UpdateBatchStruct Fields toMillisecondsThe fields
lockup_end,release_unit, andlast_claimed_release_timein theBatchstruct are updated to use theMillisecondstype. This enhances time precision and maintains consistency across the contract.Also applies to: 25-25, 30-30
55-55: Verify Index Ordering withMillisecondsEnsure that using
b.lockup_end.milliseconds()in the index correctly orders the batches as intended. SinceMillisecondsis now used, it's important to confirm that the sorting behavior remains consistent.If you'd like assistance in verifying this, please let me know.
88-88: Update Function Parameter toMillisecondsThe
current_timeparameter inget_claimable_batches_with_idsis updated toMilliseconds, ensuring consistency in time handling throughout the contract.
94-94: Review Addition of 1 Millisecond tocurrent_timeAdding 1 millisecond to
current_time.milliseconds()when calculatingmax_keymay affect batch retrieval logic. Please confirm that this adjustment correctly aligns with the intended time boundary conditions for batch eligibility.
144-170: Tests Updated to UseMillisecondsThe test cases have been appropriately updated to utilize the
Millisecondstype and associated methods likeplus_secondsandminus_seconds. This ensures that testing accurately reflects the new time unit implementations.contracts/finance/andromeda-vesting/src/contract.rs (11)
3-5: ImportMillisecondsfor time-related computations.The addition of
Millisecondsto the imports aligns with the changes to time units in the contract and enhances type safety.
119-119: Convert current time toMilliseconds.The current time is appropriately converted from seconds to
MillisecondsusingMilliseconds::from_seconds, ensuring consistency in time calculations.
138-138: Ensure non-zerorelease_unitandrelease_amount.Using
is_zero()with theMillisecondstype accurately checks for zero values, preventing invalid batch creations.
171-171: Calculatelockup_endwithMilliseconds.The
lockup_endis correctly computed by adding thelockup_durationtocurrent_timeusingplus_milliseconds, which maintains consistency in time units.
259-264: Computecurrent_timeandup_to_timeusingMilliseconds.The calculation of
current_timeand the determination ofup_to_timeusingMillisecondsensures that time comparisons are accurate and consistent throughout the function.
275-276: Calculate elapsed time and available claims accurately.The use of
minus_millisecondsand division bymilliseconds()correctly computeselapsed_timeandnum_available_claims, adhering to the updated time unit.
314-314: Convert current time toMillisecondsinclaim_batch.Consistently converting
current_timetoMillisecondsmaintains uniformity in time calculations within theclaim_batchfunction.
322-323: Calculateelapsed_timeandnum_available_claimswithMilliseconds.The calculations are appropriately adjusted to use
Milliseconds, ensuring accurate claim processing.
344-354: Updatelast_claimed_release_timewith proper overflow checks.The method of calculating
claims_release_unitincludes necessary overflow checks, and wrapping the result withMillisecondsensures type safety when updatinglast_claimed_release_time.
243-243: Update function parameter toMilliseconds.The
execute_claim_allfunction now takesup_to_timeasOption<Milliseconds>. Ensure all invocations of this function are updated to reflect the new parameter type.Run the following script to find and review function calls:
#!/bin/bash # Description: Verify all calls to `execute_claim_all` use the updated `Milliseconds` parameter. rg --type rust 'execute_claim_all' -A 5
106-107: Update function parameters to useMilliseconds.The function
execute_create_batchnow acceptslockup_durationasOption<Milliseconds>andrelease_unitasMilliseconds, which improves clarity and consistency in time representations. Ensure all calls to this function are updated to match the new signature.Run the following script to verify all function calls:
contracts/finance/andromeda-vesting/src/testing/tests.rs (25)
2-3: Imports updated appropriatelyThe imports of
amp::Recipient,withdraw::WithdrawalType, andMillisecondsare correctly added to support the updated time units and recipient handling.
40-41: Function signature updated to useMillisecondsThe
create_batchfunction now acceptslockup_duration: Option<Milliseconds>andrelease_unit: Milliseconds, enhancing type safety for time-related parameters in alignment with the PR objectives.
89-89: Test case updated withMilliseconds::from_seconds(1)In the
test_create_batch_unauthorizedtest,release_unitis correctly updated to useMilliseconds::from_seconds(1), ensuring consistency with the new time unit representation.
108-108: Test case updated withMilliseconds::from_seconds(1)The
test_create_batch_no_fundstest accurately updatesrelease_unittoMilliseconds::from_seconds(1), maintaining consistency across test cases.
131-131: Test case updated withMilliseconds::from_seconds(1)In the
test_create_batch_invalid_denomtest, therelease_unitparameter is correctly updated, reflecting the change toMilliseconds.
154-154: Validation for zero amount handled appropriatelyThe
test_create_batch_valid_denom_zero_amounttest correctly usesMilliseconds::from_seconds(1)forrelease_unit, and appropriately checks for zero amount, ensuring robustness in input validation.
172-172: Validation for zerorelease_unitThe
test_create_batch_release_unit_zerotest correctly setsrelease_unittoMilliseconds::zero(), testing the contract's handling of an invalid zero release unit.
190-190: Validation for zerorelease_amountIn the
test_create_batch_release_amount_zerotest,release_unitis properly set toMilliseconds::from_seconds(10), and the test accurately checks for a zerorelease_amount.
208-208: Test batch creation with valid parametersThe
test_create_batchtest correctly initializesrelease_unitwithMilliseconds::from_seconds(10)andrelease_amount, ensuring the batch is created with valid parameters.
213-213: Current time captured usingMillisecondsCapturing the current time with
Milliseconds::from_seconds(mock_env().block.time.seconds())enhances the precision of time tracking in tests.
220-220: Attributes updated to reflectMillisecondsThe
add_attributecall correctly convertsrelease_unitto a string usingMilliseconds::from_seconds(10).to_string(), ensuring accurate logging.
232-232: Batch struct updated withMillisecondstypesThe
Batchstruct initialization now usesMilliseconds::from_seconds(10)forrelease_unit, ensuring consistency in time representation.
243-243: Optionallockup_durationusingMillisecondsThe
lockup_durationparameter is correctly set usingSome(Milliseconds::from_seconds(100)), allowing for optional lockup durations with precise time units.
254-255: Attributes reflect adjustedlockup_endandrelease_unitThe
lockup_endandrelease_unitattributes are accurately represented usingMilliseconds, aligning with the updated time unit usage.
266-267: Batch struct correctly handleslockup_endandrelease_unitThe batch's
lockup_end,release_unit, andlast_claimed_release_timeare properly initialized withMilliseconds, ensuring accurate time tracking.Also applies to: 269-269
295-296: Batch creation withMillisecondsin multi-batch unsupported testIn
test_create_batch_multi_batch_not_supported,lockup_durationandrelease_unitare correctly set usingMilliseconds, complying with the new time unit standards.
301-301: Current time captured usingMillisecondsConsistently capturing the current time enhances test reliability and accuracy.
307-308: Attributes updated withMillisecondsvaluesThe
add_attributecalls includelockup_endandrelease_unitformatted as strings, reflecting the updated time units.
319-320: Batch initialization with updated time unitsThe batch in
test_create_batch_multi_batch_not_supportedis initialized withMillisecondsforlockup_end,release_unit, andlast_claimed_release_time.Also applies to: 322-322
360-361: Batch creation withlockup_durationusingMillisecondsThe test
test_claim_batch_still_lockedcorrectly setslockup_durationtoSome(Milliseconds::from_seconds(100)), verifying lockup behavior with the new time units.
387-387: Test case update withMillisecondsforrelease_unitIn
test_claim_batch_no_funds_available,release_unitis properly updated toMilliseconds::from_seconds(10).
603-605: Batch struct correctly reflects updatedrelease_unitandlast_claimed_release_timeThe
Batchstruct in the test accurately usesMillisecondsforrelease_unitand updateslast_claimed_release_time, ensuring consistent time tracking.
729-729: Test correctly advances time usingrelease_unitIn
test_claim_batch_multiple_claims, time is advanced usingenv.block.time.plus_seconds(4 * release_unit.seconds()), which aligns with theMillisecondsusage.
783-783: Batch struct updated after multiple claimsThe
Batchstruct correctly updatesamount_claimedandlast_claimed_release_timeafter multiple claims, ensuring correct state management.
993-993: Batch responses accurately reflectMillisecondsusageIn
test_claim_all, theBatchResponsestructures correctly represent time-related fields usingMilliseconds, enhancing consistency.
| @@ -411,7 +414,7 @@ fn get_batch_response( | |||
| ) -> Result<BatchResponse, ContractError> { | |||
| let previous_amount = batch.amount_claimed; | |||
| let previous_last_claimed_release_time = batch.last_claimed_release_time; | |||
| let amount_available_to_claim = if env.block.time.seconds() >= batch.lockup_end { | |||
| let amount_available_to_claim = if env.block.time.seconds() >= batch.lockup_end.seconds() { | |||
There was a problem hiding this comment.
Ensure consistent time units in conditional checks.
Comparing env.block.time.seconds() with batch.lockup_end.seconds() might introduce precision issues due to truncation. Consider converting env.block.time to Milliseconds for an accurate comparison.
Apply this change to improve precision:
-let amount_available_to_claim = if env.block.time.seconds() >= batch.lockup_end.seconds() {
+let current_time = Milliseconds::from_seconds(env.block.time.seconds());
+let amount_available_to_claim = if current_time >= batch.lockup_end {Committable suggestion was skipped due to low confidence.
| @@ -406,7 +408,7 @@ fn test_claim_batch_single_claim() { | |||
| init(deps.as_mut()); | |||
| let info = mock_info("owner", &coins(100, "uusd")); | |||
|
|
|||
| let release_unit = 10; | |||
| let release_unit = Milliseconds::from_seconds(10); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider defining release_unit as a constant
The release_unit is repeatedly defined as Milliseconds::from_seconds(10) across multiple test functions. To improve maintainability and reduce duplication, consider defining release_unit as a constant at the module level.
Apply this diff to define RELEASE_UNIT as a constant:
+const RELEASE_UNIT: Milliseconds = Milliseconds::from_seconds(10);
// In test functions, replace:
-let release_unit = Milliseconds::from_seconds(10);
+// Use the constant:
+let release_unit = RELEASE_UNIT;This change promotes consistency and simplifies updates to the release_unit value in the future.
Also applies to: 492-492, 645-645, 711-711, 804-804, 870-870, 947-947
| @@ -569,7 +571,7 @@ | |||
| // Skip time. | |||
| let mut env = mock_env(); | |||
| // Two releases are out. | |||
| env.block.time = env.block.time.plus_seconds(2 * release_unit); | |||
| env.block.time = env.block.time.plus_seconds(2 * release_unit.seconds()); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust time advancement using Milliseconds
In test_claim_batch_not_nice_numbers_multiple_releases, consider using release_unit directly for time advancement to maintain type consistency.
Apply this diff to update the time advancement:
-env.block.time = env.block.time.plus_seconds(2 * release_unit.seconds());
+env.block.time = env.block.time.plus_milliseconds(2 * release_unit.milliseconds());This ensures that time calculations remain consistent with the Milliseconds type throughout the test.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| env.block.time = env.block.time.plus_seconds(2 * release_unit.seconds()); | |
| env.block.time = env.block.time.plus_milliseconds(2 * release_unit.milliseconds()); |
| let release_unit = Milliseconds::from_seconds(1); // 1 second | ||
| let duration: u64 = 60 * 60 * 24 * 365 * 5; // 5 years |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using Milliseconds for duration
Currently, duration is defined as a u64. To maintain type consistency and enhance readability, consider defining duration as a Milliseconds.
Apply this diff to update duration:
-let duration: u64 = 60 * 60 * 24 * 365 * 5; // 5 years
+let duration = Milliseconds::from_seconds(60 * 60 * 24 * 365 * 5); // 5 yearsAdjust subsequent code to use duration.seconds() or duration.milliseconds() as needed.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let release_unit = Milliseconds::from_seconds(1); // 1 second | |
| let duration: u64 = 60 * 60 * 24 * 365 * 5; // 5 years | |
| let release_unit = Milliseconds::from_seconds(1); // 1 second | |
| let duration = Milliseconds::from_seconds(60 * 60 * 24 * 365 * 5); // 5 years |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- CHANGELOG.md (1 hunks)
- contracts/finance/andromeda-vesting/src/contract.rs (12 hunks)
- contracts/finance/andromeda-vesting/src/mock.rs (0 hunks)
- contracts/finance/andromeda-vesting/src/state.rs (6 hunks)
- contracts/finance/andromeda-vesting/src/testing/tests.rs (51 hunks)
- packages/andromeda-finance/src/vesting.rs (3 hunks)
💤 Files with no reviewable changes (1)
- contracts/finance/andromeda-vesting/src/mock.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🔇 Additional comments (25)
contracts/finance/andromeda-vesting/src/contract.rs (10)
3-5: LGTM: Import ofMillisecondstypeThe addition of the
Millisecondsimport fromandromeda_std::commonis consistent with the PR objective of converting seconds to milliseconds. This change lays the foundation for using the new time unit throughout the contract.
105-106: LGTM: Updated function signature to useMillisecondsThe changes to the
execute_create_batchfunction signature, specifically updatinglockup_durationtoOption<Milliseconds>andrelease_unittoMilliseconds, are in line with the PR objective. This ensures consistency in time unit usage throughout the contract.
118-118: LGTM: Conversion of block time to millisecondsThe calculation of
current_timeusingMilliseconds::from_seconds(env.block.time.seconds())correctly converts the block time from seconds to milliseconds. This change ensures consistency with the new time unit used throughout the contract.
137-137: LGTM: Updated zero check forMillisecondstypeThe zero amount check has been correctly updated to use the
is_zero()method on theMillisecondstype. This change maintains the existing logic while adapting to the new time unit representation.
170-170: LGTM: Updated time addition usingplus_millisecondsThe use of
plus_millisecondsfor adding the lockup duration to the current time is correct and consistent with the newMillisecondstype. This ensures accurate calculation of the lockup end time.
242-242: LGTM: Updatedexecute_claim_allto useMillisecondsThe changes in the
execute_claim_allfunction, including the parameter type update and theup_to_timecalculation, correctly implement the conversion to milliseconds. The logic remains consistent while adapting to the new time unit representation.Also applies to: 258-263
274-275: LGTM: Updated time calculations to useMillisecondsmethodsThe use of
minus_millisecondsfor time subtraction and the adaptation of thenum_available_claimscalculation to usemilliseconds()are correct and consistent with the newMillisecondstype. These changes ensure accurate time-based calculations in the contract.
313-313: LGTM: Updatedclaim_batchfunction to useMillisecondsThe changes in the
claim_batchfunction correctly implement the conversion to milliseconds. The use ofMillisecondsmethods for time calculations and the added overflow check enhance the accuracy and safety of the time-based operations in the contract.Also applies to: 321-322, 343-353
Line range hint
1-441: Summary: Successful implementation of milliseconds conversionThe changes in this file successfully implement the conversion from seconds to milliseconds as per the PR objective. The use of the
Millisecondstype is consistent throughout the contract, ensuring improved precision in time-based calculations and comparisons. All modified segments have been reviewed and approved.One existing precision issue was noted and a suggestion for improvement has been provided. Overall, the changes enhance the contract's handling of time-related operations and align with the stated goals of the pull request.
416-416:⚠️ Potential issueConsider addressing the precision issue in time comparison
There's an existing potential precision issue in the comparison between
env.block.time.seconds()andbatch.lockup_end.seconds(). This comparison might introduce inaccuracies due to the loss of millisecond precision when converting to seconds.Consider updating this comparison to use milliseconds for improved precision:
-let amount_available_to_claim = if env.block.time.seconds() >= batch.lockup_end.seconds() { +let current_time = Milliseconds::from_seconds(env.block.time.seconds()); +let amount_available_to_claim = if current_time >= batch.lockup_end {This change would ensure consistent precision in time comparisons throughout the contract.
packages/andromeda-finance/src/vesting.rs (5)
2-4: ImportMillisecondsfor time precisionAdding
Millisecondsto the imports ensures that time-related parameters throughout the contract use a consistent and precise type, enhancing readability and reducing potential errors.
34-34: Updateup_to_timeto useMillisecondsUpdating
up_to_timetoOption<Milliseconds>in theClaimAllmessage enhances type safety and aligns with the contract's use of milliseconds for time measurements.
40-42: Consistent time units inCreateBatchChanging
lockup_durationandrelease_unittoMillisecondsin theCreateBatchmessage improves consistency across the contract and ensures precise time calculations.
88-95: UseMillisecondsinBatchResponsefieldsUpdating
lockup_end,release_unit, andlast_claimed_release_timetoMillisecondsin theBatchResponsestruct ensures that all time-related fields use the same unit, improving consistency and reducing potential confusion.
Line range hint
34-95: Verify all time-related types are updated toMillisecondsWhile the update to
Millisecondsenhances precision, please ensure that all references to these fields throughout the codebase have been updated accordingly to prevent type mismatches or serialization issues.Run the following script to identify any remaining usages of
u64for time-related parameters:contracts/finance/andromeda-vesting/src/state.rs (7)
2-5: Imports updated to include necessary modulesThe imports have been correctly updated to include
WithdrawalTypeandMillisecondsfromandromeda_std::common, which are required for the updated time handling and withdrawal logic.
7-7: Usage ofcosmwasm_stdimports is appropriateThe import statement for
cosmwasm_std::{Order, Storage, Uint128}has been updated appropriately to support the changes in the code.
23-23: Time fields inBatchstruct updated toMillisecondsThe fields
lockup_end,release_unit, andlast_claimed_release_timein theBatchstruct have been updated to use theMillisecondstype. This enhances time precision and consistency across the contract.Also applies to: 25-25, 30-30
55-55: Indexing adjusted forMillisecondstypeIn the
claim_timeindex, the code now usesb.lockup_end.milliseconds(). This ensures that the index operates correctly with the newMillisecondstype by accessing the underlying millisecond value.
137-163: Tests updated to reflectMillisecondschangesIn the
test_get_claimable_batches_with_idsfunction,current_timeis correctly initialized usingMilliseconds::from_seconds. Time fields in the test batches have been updated to useMilliseconds, and methods likeplus_secondsandminus_secondsare used appropriately.
81-87:get_claimable_batches_with_idsnow usesMillisecondsThe
get_claimable_batches_with_idsfunction has been updated to acceptcurrent_timeasMilliseconds. The calculation ofmax_keyhas been adjusted to usecurrent_time.milliseconds() + 1, ensuring correct comparison logic.Run the following script to check all calls to
get_claimable_batches_with_ids:✅ Verification successful
All calls to
get_claimable_batches_with_idscorrectly passMillisecondsas thecurrent_timeparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all calls to get_claimable_batches_with_ids pass Milliseconds rg --type rust 'get_claimable_batches_with_ids\(' -A 3 -B 3Length of output: 2668
64-64:⚠️ Potential issueFunction
save_new_batchsignature modifiedThe
save_new_batchfunction no longer accepts theconfigparameter. Sinceconfigwas previously used to check if multi-batch functionality was enabled, please ensure that this change aligns with the intended functionality and that any related logic has been appropriately refactored.Run the following script to find and review all calls to
save_new_batch:✅ Verification successful
Function
save_new_batchsignature modification verifiedAll usages of
save_new_batchhave been updated to match the new function signature without theconfigparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all usages of save_new_batch to verify function calls rg --type rust 'save_new_batch\(' -A 3 -B 3Length of output: 1174
contracts/finance/andromeda-vesting/src/testing/tests.rs (3)
2-3: Imports updated appropriately to include necessary modulesThe addition of
amp::Recipientandcommon::{withdraw::WithdrawalType, Milliseconds}ensures that the necessary types are available for the updated code.
39-40: Function signature updated to enhance type safetyThe
create_batchfunction now acceptslockup_duration: Option<Milliseconds>andrelease_unit: Milliseconds, which improves type safety and consistency in time-related parameters.
87-87: Consistent use ofMilliseconds::from_secondsin test casesUsing
Milliseconds::from_seconds(...)forrelease_unitin test cases enhances readability and maintains consistency across the codebase.Also applies to: 106-106, 129-129, 152-152, 170-170, 188-188, 206-206
| @@ -663,14 +605,14 @@ fn test_claim_batch_middle_of_interval() { | |||
|
|
|||
| let mut env = mock_env(); | |||
| // Only halfway to first release. | |||
| env.block.time = env.block.time.plus_seconds(release_unit / 2); | |||
| env.block.time = env.block.time.plus_seconds(release_unit.seconds() / 2); | |||
There was a problem hiding this comment.
Potential integer division leading to zero time advancement
Dividing release_unit.seconds() by 2 may cause integer truncation, resulting in zero when release_unit.seconds() is an odd number (e.g., 1 second). This could lead to the block time not advancing as intended.
Apply this diff to use milliseconds for precise time advancement:
-env.block.time = env.block.time.plus_seconds(release_unit.seconds() / 2);
+env.block.time = env.block.time.plus_milliseconds(release_unit.milliseconds() / 2);📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| env.block.time = env.block.time.plus_seconds(release_unit.seconds() / 2); | |
| env.block.time = env.block.time.plus_milliseconds(release_unit.milliseconds() / 2); |
| @@ -952,17 +894,22 @@ | |||
| create_batch( | |||
| deps.as_mut(), | |||
| None, | |||
| release_unit / 2, | |||
| Milliseconds::from_seconds(release_unit.seconds() / 2), | |||
There was a problem hiding this comment.
Avoid loss of precision when creating half release_unit
Using integer division in Milliseconds::from_seconds(release_unit.seconds() / 2) may lead to unintended zero or incorrect duration due to truncation. To maintain accuracy, calculate using milliseconds.
Apply this diff to correctly compute half of release_unit:
-create_batch(
deps.as_mut(),
None,
- Milliseconds::from_seconds(release_unit.seconds() / 2),
+ Milliseconds::from_milliseconds(release_unit.milliseconds() / 2),
release_amount.clone(),
);Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
contracts/finance/andromeda-vesting/src/testing/tests.rs (4)
Line range hint
202-263: LGTM: Time-related parameters updated to useMillisecondsThe changes in this test function correctly implement the use of
Millisecondsfor time-related parameters and calculations. The addition ofcurrent_timeusingMillisecondsis a good practice for consistency.Consider extracting the
Milliseconds::from_seconds(10)into a constant at the beginning of the test function to improve readability and maintainability. For example:let release_unit = Milliseconds::from_seconds(10);Then use
release_unitthroughout the test instead of repeatingMilliseconds::from_seconds(10).
Line range hint
490-569: LGTM with suggestion: Consistent use ofMilliseconds, butdurationcan be improvedThe changes in this test function correctly implement the use of
Millisecondsfor most time-related parameters and calculations. However, there's an opportunity for improvement:Consider updating the
durationvariable to useMillisecondsfor consistency:-let duration: u64 = 60 * 60 * 24 * 365 * 5; // 5 years +let duration = Milliseconds::from_seconds(60 * 60 * 24 * 365 * 5); // 5 yearsThen, update subsequent uses of
durationto call.seconds()or.milliseconds()as needed.
Line range hint
581-635: LGTM with suggestion: Use ofMillisecondsis consistent, but division might lose precisionThe changes in this test function correctly implement the use of
Millisecondsfor time-related parameters and calculations. However, there's a potential issue:On line 604, the division
release_unit.seconds() / 2might lead to precision loss ifrelease_unit.seconds()is an odd number. Consider using milliseconds for more precise calculations:-env.block.time = env.block.time.plus_seconds(release_unit.seconds() / 2); +env.block.time = env.block.time.plus_milliseconds(release_unit.milliseconds() / 2);This ensures that you're always advancing time by exactly half of the release unit, even if it's an odd number of seconds.
Line range hint
883-974: LGTM with suggestion: Consistent use ofMilliseconds, but potential precision lossThe changes in this test function correctly implement the use of
Millisecondsfor time-related parameters and calculations, maintaining consistency with the updated type system. However, there's a potential issue:On line 893, creating a batch with half of the
release_unitusingMilliseconds::from_seconds(release_unit.seconds() / 2)might lead to precision loss ifrelease_unit.seconds()is an odd number. Consider using milliseconds for more precise calculations:-Milliseconds::from_seconds(release_unit.seconds() / 2), +Milliseconds::from_milliseconds(release_unit.milliseconds() / 2),This ensures that you're always using exactly half of the release unit, even if it's an odd number of seconds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- contracts/finance/andromeda-vesting/src/mock.rs (1 hunks)
- contracts/finance/andromeda-vesting/src/testing/tests.rs (51 hunks)
- packages/andromeda-finance/src/vesting.rs (3 hunks)
- packages/std/src/error.rs (0 hunks)
💤 Files with no reviewable changes (1)
- packages/std/src/error.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/andromeda-finance/src/vesting.rs
🧰 Additional context used
🔇 Additional comments (12)
contracts/finance/andromeda-vesting/src/mock.rs (2)
27-27: LGTM: Changes align with PR objectivesThe removal of
is_multi_batch_enabledandunbonding_durationparameters from theinstantiatemethod and the corresponding update to themock_vesting_instantiate_msgcall align well with the PR objectives. This change simplifies the contract instantiation process and removes the unnecessary batch flag as intended.
Line range hint
45-55: LGTM: Consistent changes, verify test coverageThe updates to
mock_vesting_instantiate_msgfunction are consistent with the changes made to theinstantiatemethod. Removingis_multi_batch_enabledandunbonding_durationparameters aligns with the PR objectives to simplify the contract and remove the unnecessary batch flag.To ensure these changes don't negatively impact existing functionality:
Please review the test results to ensure all tests are passing and that there are no unexpected references to the removed parameters.
contracts/finance/andromeda-vesting/src/testing/tests.rs (10)
3-3: LGTM: Import ofMillisecondsaddedThe addition of
Millisecondsimport is consistent with the PR objectives to use milliseconds instead of seconds for time-related parameters.
35-36: LGTM: Function signature updated to useMillisecondsThe changes to the
create_batchfunction signature, replacingu64withMillisecondsforlockup_durationandrelease_unit, are in line with the PR objectives to use milliseconds for time-related parameters.
166-166: LGTM:release_unitupdated to useMilliseconds::zero()The change from
release_unit: 0torelease_unit: Milliseconds::zero()correctly implements the newMillisecondstype for therelease_unitparameter, maintaining consistency with the updated type system.
Line range hint
347-416: LGTM: Consistent use ofMillisecondsfor time-related operationsThe changes in this test function correctly implement the use of
Millisecondsfor time-related parameters and calculations. The addition ofrelease_unitas a variable improves readability and consistency throughout the test.
Line range hint
428-477: LGTM: Consistent implementation ofMillisecondsfor time calculationsThe changes in this test function correctly implement the use of
Millisecondsfor time-related parameters and calculations, maintaining consistency with the updated type system throughout the test.
Line range hint
647-728: LGTM: Consistent implementation ofMillisecondsfor time calculationsThe changes in this test function correctly implement the use of
Millisecondsfor time-related parameters and calculations. The use ofrelease_unitimproves readability and maintains consistency with the updated type system throughout the test.
Line range hint
740-789: LGTM: Consistent use ofMillisecondsfor time-related operationsThe changes in this test function correctly implement the use of
Millisecondsfor time-related parameters and calculations. The addition ofrelease_unitas a variable improves readability and maintains consistency with the updated type system throughout the test.
Line range hint
806-855: LGTM: Consistent implementation ofMillisecondsfor time calculationsThe changes in this test function correctly implement the use of
Millisecondsfor time-related parameters and calculations. The use ofrelease_unitimproves readability and maintains consistency with the updated type system throughout the test.
Line range hint
1001-1033: LGTM: Consistent implementation ofMillisecondsfor time calculationsThe changes in this latter part of the
test_claim_allfunction correctly implement the use ofMillisecondsfor time-related parameters and calculations. This maintains consistency with the updated type system throughout the test.
Line range hint
1-1033: Overall assessment: Successfully implementedMillisecondswith minor suggestions for improvementThe changes in this file successfully implement the use of
Millisecondsfor time-related parameters and calculations throughout the test suite. This aligns well with the PR objectives to use milliseconds instead of seconds for better precision and consistency with other ADOs.Key points:
- All time-related parameters now use the
Millisecondstype.- Time calculations and comparisons have been updated to work with
Milliseconds.- The changes maintain consistency across all test functions.
Minor suggestions for improvement:
- Consider using a constant for repeated
Milliseconds::from_seconds(10)in some test functions.- Update the
durationvariable intest_claim_batch_not_nice_numbers_multiple_releasesto useMilliseconds.- Be cautious of potential precision loss when dividing seconds, especially when creating half intervals. Consider using milliseconds for these calculations.
These changes significantly enhance the handling of time units in the vesting contract's testing framework, improving type safety and precision.
Motivation
Resolves following issues
Implementation
Millisecondsforup_to_time,lockup_durationandrelease_unitinstead ofu64Testing
Unit tests for vesting updated to use
MillisecondsRemoved test case with disabled multi batching option
Summary by CodeRabbit
New Features
Bug Fixes
Changes