fix(contract): remove redundant TotalByAsset storage key - #1082
Merged
OlaGreat merged 2 commits intoAug 31, 2026
Merged
Conversation
|
@precious-akpan 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! 🚀 |
…reat#1038) RecipientTotal and TotalByAsset were both keyed by (recipient, asset) and updated with the exact same increment/decrement in support() and withdraw(), so every support() call wrote two persistent ledger entries (plus two extend_ttl calls) holding the same value — doubling storage rent for zero informational benefit. Drop the TotalByAsset key and its writes; withdraw()'s existence/balance check now reads RecipientTotal directly. get_total_by_asset() becomes a thin wrapper over get_recipient_total() so its public behavior (and the ABI.md docs, which already described it as mirroring RecipientTotal) is unchanged. Test snapshots updated to reflect the now-smaller ledger footprint (one fewer contract_data read/write entry per support()/withdraw() call).
's reentrancy fix main's Contract CI has been red since the OlaGreat#1040 fix merged: it moved the external token balance() check to the very start of withdraw(), ahead of the RecipientNotFound/ZeroBalance/WithdrawAmountExceedsBalance checks — correct per OlaGreat#1040 (checks-effects-interactions), but it left over_withdraw, withdraw_with_no_balance, and withdraw_again_after_full_withdrawal_... asserting the wrong error: their scenarios also happen to leave the contract's real token balance below the requested amount, so InsufficientContractBalance now fires first instead of the app-level error each test means to exercise. Fund the contract directly via the token admin (bypassing support()) in each test, so the external balance() check passes and the intended application-level check is what actually gets exercised.
precious-akpan
force-pushed
the
fix/1038-remove-redundant-storage-key
branch
from
August 31, 2026 00:17
7788f22 to
418c641
Compare
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
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
RecipientTotal(Address, Address)andTotalByAsset(Address, Address)were both keyed by (recipient, asset) and updated with the exact same increment/decrement insupport()/withdraw().support()call wrote two persistent ledger entries (plus twoextend_ttlcalls) holding the same value, doubling storage rent/fees for zero informational benefit.TotalByAssetDataKeyvariant and its writes insupport().withdraw()'s existence/balance check and decrement now operate onRecipientTotaldirectly (previously usedTotalByAssetfor this).get_total_by_asset()is now a thin wrapper overget_recipient_total()— public behavior is unchanged, and this matchescontract/ABI.md's existing description ofget_recipient_totalas "mirror[ing]get_total_by_assetand read[ing] from the sameRecipientTotalstorage key."DataKey::TotalByAssetdirectly in storage to useDataKey::RecipientTotalinstead, since that's now the keywithdraw()reads.Closes #1038
Also fixed: 3 pre-existing test failures unrelated to #1038, blocking this PR's CI
main's Contract CI has been red since #1040's reentrancy fix merged (moves the externalbalance()check to the very start ofwithdraw(), ahead of theRecipientNotFound/ZeroBalance/WithdrawAmountExceedsBalancechecks — correct per #1040's design, but it leftover_withdraw,withdraw_with_no_balance, andwithdraw_again_after_full_withdrawal_...asserting the wrong error, since their scenarios also happen to leave the contract's real token balance below the requested amount). Added a second commit that funds the contract directly via the token admin (bypassingsupport()) in each of those 3 tests, so the external balance check passes and the intended application-level check is what actually gets exercised. Confirmed this reproduces identically on unmodifiedmain(i.e., it's not something my#1038change introduced).Test plan
cargo test(contract/contracts/support_page): 27/27 passed (previously 24 passed / 3 pre-existing failures — see above).cargo clippy -p support_page --all-targets— no new warnings introduced (2 pre-existinglen_zerowarnings, untouched by this diff).contract_dataledger entry (the removedTotalByAssetread/write) persupport()/withdraw()call.