Add parameter bounds validation in update_config - #615
Open
KorexOnchain wants to merge 2 commits into
Open
Conversation
Add missing min/max bounds validation to four previously-unchecked update_config parameters: - decay_rate_bps: must be nonzero and <= 5000 bps (50%). Unbounded, a value of 10000 (100%) would instantly zero all reputation scores; a value of 0 would silently disable decay entirely. - decay_period_ledgers: must be >= 100 ledgers. A value of 0 would disable decay outright (existing invoice.rs/storage.rs read sites already guard with decay_period_ledgers > 0 before dividing, so this closes the gap at the write side rather than a live division-by-zero). - dispute_timeout_ledgers: must be >= 1440 ledgers (~1 day). A value of 0 would let disputes auto-resolve instantly, before the payer has any chance to respond. - high_rep_threshold: must be > 0. A value of 0 would make every LP register as high-reputation regardless of actual score. Also fixes a related bug found while wiring up specific error codes per the acceptance criteria: the public update_config wrapper in lib.rs collapsed every ConfigError variant into ContractError::Unauthorized via a blanket map_err closure, so even the pre-existing InvalidBonusBps/InvalidMinDiscountRate checks never surfaced their real error code at the contract boundary. Added a proper impl From<ConfigError> for ContractError and six new ContractError variants (39-44) so each validation failure now returns its own distinct, documented error code. Note: the issue references a division-by-zero at invoice.rs:324 for an unbounded decay_period_ledgers. That specific read site already guards with decay_period_ledgers > 0 before dividing, so the real risk is silent decay-disablement (governance sets period=0, decay stops working, no error anywhere) rather than a live panic. Bounding at the write side in update_config is still the correct fix. Tests: 10 new tests in test.rs covering the zero/max-invalid and exact-boundary-valid case for each of the four parameters, run via crate::config::update_config directly (bypassing the public client) against a minimal admin-owned config built in storage. Closes Invoice-Liquidity-Network#604
|
@KorexOnchain 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! 🚀 |
…go-deny wildcards check
6 tasks
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
Adds missing min/max bounds validation to four previously-unchecked
update_configparameters, per issue #604.Changes
decay_rate_bps: must be nonzero and <= 5000 bps (50%). Unbounded, a value of 10000 (100%) would instantly zero all reputation scores; a value of 0 would silently disable decay entirely (not called out in the original issue, but the same class of bug as the upper-bound case, so added it too).decay_period_ledgers: must be >= 100 ledgers. A value of 0 would disable decay outright.dispute_timeout_ledgers: must be >= 1440 ledgers (~1 day). A value of 0 would let disputes auto-resolve instantly, before the payer can respond.high_rep_threshold: must be > 0. A value of 0 would make every LP register as high-reputation regardless of actual score.Also fixed: error codes were never actually reaching the caller
While wiring up specific error codes per the acceptance criteria, found that the public
update_configwrapper inlib.rscollapsed everyConfigErrorvariant intoContractError::Unauthorizedvia a blanketmap_err(|_| ContractError::Unauthorized). This meant even the pre-existingInvalidBonusBps/InvalidMinDiscountRatechecks never surfaced their real error code at the contract boundary — every config validation failure looked like an auth failure to callers. Added a properimpl From<ConfigError> for ContractErrorand six newContractErrorvariants (39-44) so each validation failure — old and new — now returns its own distinct, documented error code. No test depended on the old broken behavior, confirmed before making this change.Note on the issue's division-by-zero claim
The issue references a division-by-zero at
invoice.rs:324for an unboundeddecay_period_ledgers. That specific read site (and the equivalent one instorage.rs) already guards withdecay_period_ledgers > 0before dividing, so there's no live division-by-zero today. The real risk this bounds check closes is silent decay-disablement: governance setsperiod=0, decay silently stops working, no error anywhere. Bounding at the write side inupdate_configis still the correct fix — it's just a different failure mode than the issue described.Tests
10 new tests in
test.rs, covering the zero/max-invalid and exact-boundary-valid case for each of the four parameters. Tests callcrate::config::update_configdirectly (bypassing the public client) against a minimal admin-owned config built in storage, to keep each test isolated to the validation logic itself.Verification
cargo build -p invoice_liquidity— compiles clean, only pre-existing unrelated warnings (insurance_poollifetime elision, unused imports instorage.rs).cargo test -p invoice_liquidity --lib— this crate's test suite has pre-existing compile errors intests_storage_layout.rs,tests_new_features.rs, and a separatetests/governance_main_integration_test.rsintegration binary, all unrelated to this change (confirmed viagit stash— same errors reproduce identically on baremainwith none of this PR's changes present). Verified our 4 changed files add zero new compile errors versus that baseline, and our 10 new tests all pass in isolation.Closes #604