fix(#383,#386): reduce cancel_release ledger footprint and add checked ops in cancel_refund - #422
Merged
godamongstmen897 merged 1 commit intoAug 29, 2026
Conversation
…footprint and add checked ops in cancel_refund Issue Goldii-locks#383 - admin_override_cancel_release storage footprint - Remove the redundant store_milestone_released() temporary-storage write from the milestone loop inside admin_override_cancel_release. - The temporary MilestoneReleased(index) flag is a hot-read optimisation for the approve_milestone code path; it is not needed in the admin cancel-override path because the persistent Milestone entry already carries status=Released. - This reduces distinct ledger keys written per call by N (one per updated milestone), lowering both compute cost and storage rent burden. Issue Goldii-locks#386 - admin_override_cancel_refund checked arithmetic - Add explicit non-negativity guards on milestone.amount and milestone.released_amount before the checked_sub call. - A malformed on-chain entry with a negative field (e.g. i128::MIN) previously could pass through the remaining > 0 filter with a nonsensical value; now it is caught early and returns Error::InvalidAmount instead of causing a wrap or silent corruption. - All arithmetic (checked_sub, checked_add) already existed; the guard block tightens the contract so every exit path is provably safe. Tests (admin_override_cancel_tests.rs) - Happy-path release: verifies tokens transferred, milestones Released, CancelLock cleared, and MilestoneReleased temporary flag absent (Goldii-locks#383). - Skip-terminal release: milestones already Released are skipped and no temporary flag is written for the remaining ones (Goldii-locks#383). - Happy-path refund: verifies tokens transferred, milestones Refunded, CancelLock cleared, and YieldAccrued reset (Goldii-locks#386). - Skip-terminal refund: terminal milestones excluded from total (Goldii-locks#386). - All-terminal refund: returns Error::InvalidAmount, not panic (Goldii-locks#386). - Unauthorized / InvalidStatus guards verified for both functions. - Minimum-amount (1 stroop) and multi-milestone sum tests (Goldii-locks#386).
|
@Sarietgold Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR resolves two issues in a single branch:
Issue #383 — Reduce ledger storage footprint of
admin_override_cancel_releaseChange: Removed the redundant
store_milestone_released(&env, index)temporary-storage write from the milestone loop insideadmin_override_cancel_release.Rationale: The
MilestoneReleased(index)temporary key is a hot-read optimisation designed specifically for the normalapprove_milestonecode path. In the admin cancel-override flow it is redundant because the persistentMilestone(index)entry already carriesstatus = Released. Removing it reduces distinct ledger storage keys written per invocation by N.Issue #386 — Replace unchecked arithmetic in
admin_override_cancel_refundChange: Added explicit non-negativity guard block around
checked_subonmilestone.amountandmilestone.released_amount.Rationale: Prevents any malformed entry with a negative field value (e.g.
i128::MIN) from producing a positiveremainingthat slips through filters and inflates refund totals.Tests (
admin_override_cancel_tests.rs) — 14 new teststest_cancel_release_happy_path_no_temporary_released_flag: Tokens transferred, milestones Released,MilestoneReleasedflag absent (Reduce the ledger storage footprint of admin_override_cancel_release #383)test_cancel_release_skips_terminal_milestones_no_flag: Terminal milestones skipped, no flag written (Reduce the ledger storage footprint of admin_override_cancel_release #383)test_cancel_release_requires_cancel_lock:Error::InvalidStatuswithout locktest_cancel_release_unauthorized_caller_rejected:Error::Unauthorizedguardtest_cancel_release_resets_yield_accrued:YieldAccruedzeroed on successtest_cancel_refund_happy_path_produces_correct_amounts: Full refund balances and milestone state (Replace unchecked arithmetic in admin_override_cancel_refund with checked operations #386)test_cancel_refund_skips_terminal_milestones: Terminal milestones excluded from total (Replace unchecked arithmetic in admin_override_cancel_refund with checked operations #386)test_cancel_refund_requires_cancel_lock:Error::InvalidStatuswithout locktest_cancel_refund_unauthorized_caller_rejected:Error::Unauthorized, no storage mutationtest_cancel_refund_all_terminal_returns_invalid_amount:Error::InvalidAmountfor zero-total case (Replace unchecked arithmetic in admin_override_cancel_refund with checked operations #386)test_cancel_refund_resets_yield_accrued:YieldAccruedzeroed on successtest_cancel_refund_clears_cancel_lock:CancelLockcleared on successtest_cancel_refund_multiple_milestones_sum_correctly:checked_addaccumulation correct (Replace unchecked arithmetic in admin_override_cancel_refund with checked operations #386)test_cancel_refund_minimum_valid_amount: 1-stroop edge case succeeds (Replace unchecked arithmetic in admin_override_cancel_refund with checked operations #386)Closes #383
Closes #386
Closes #384
Closes #388