Repository navigation
fix: harden keeper, product-name and confidence validation (#569 #570 #571 #572) - #659
Merged
nonsobethel0-dev merged 1 commit intoSep 27, 2026
Conversation
…d-Protocol#569 Parashield-Protocol#570 Parashield-Protocol#571 Parashield-Protocol#572) Closes Parashield-Protocol#569 Closes Parashield-Protocol#570 Closes Parashield-Protocol#571 Closes Parashield-Protocol#572 Parashield-Protocol#572 - add_keeper never validated the keeper address `initialize` validates the four addresses it wires up, but `add_keeper` - the entry point that actually mints settlement authority - did not. A keeper can move coverage USDC out of the pool, so a malformed StrKey written into the registry produces an entry that can never be legitimately exercised and that indexers fail to resolve when reconciling `keeper_added`. It now applies the same check. `remove_keeper` deliberately still does not, so an admin can evict a legacy malformed entry instead of being locked out of a broken registry. The rule itself moves into a pure `is_well_formed_strkey` helper (exact 56-byte length, `G`/`C` version byte, base32 body) so it can be unit-tested against byte strings no `Address` could ever be built from. The CRC16 checksum is intentionally not re-derived: the host already guarantees a canonical encoding, so that branch is unreachable and would only cost gas. Parashield-Protocol#570 - deprecate_product leaked the product-name index `create_product` rejects a name that is still mapped, and the `(category, oracle_key)` slot was released on retirement but the name was not. Retiring a product therefore burned its name permanently: the admin could never re-launch the same product line, and users saw "that name is taken" for a product that no longer existed. Retirement now releases the name, and only when the entry still points at the product being retired, so a stale mapping cannot be dropped on another's behalf. Parashield-Protocol#571 - confidence bounds were unpinned The `[1,100]` check existed on all four submission entry points but only `submit_data`'s boundaries were covered, and nothing pinned that the two batch paths and the encrypted path reject the same values or leave nothing behind when they do. Parashield-Protocol#569 - one-claim-per-policy was unpinned The `PolicyClaim` index already gated every creation path, but nothing held it: no test covered a repeated id inside one batch, a claim filed by `auto_process` blocking a later `submit_claim`, or a refused duplicate consuming a claim id. Adds 39 tests. Each new test was verified to fail against a mutated build with the corresponding check removed (7 for Parashield-Protocol#571, 7 for Parashield-Protocol#569, 4 for Parashield-Protocol#570), so the coverage is load-bearing rather than decorative.
|
@Lekan101 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
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.
Closes #569
Closes #570
Closes #571
Closes #572
Summary
Four validation/robustness issues closed in one change. Two were real defects; two were invariants that existed but were not pinned by any test, so nothing stopped them regressing.
add_keepernow validates the keeper addressdeprecate_productreleased the oracle-key slot but leaked the name index[1,100]confidence check on all four submission paths#572 -
add_keepernever validated the keeper addressinitializerunsvalidate_stellar_addresson the four addresses it wires up, butadd_keeper- the entry point that actually mints settlement authority - did not. A keeper can move coverage USDC out of the pool, so a malformed StrKey written into the registry is an entry that can never be legitimately exercised and that off-chain indexers fail to resolve when reconcilingkeeper_addedevents. It now applies the same check.remove_keeperdeliberately still does not re-run the check. That is the remediation path: an admin has to be able to evict a keeper admitted before the check existed, otherwise a malformed entry would be permanently unremovable.The rule itself moved out of the
Addresscall site into a pureis_well_formed_strkeyhelper, so it is one testable function rather than a length-and-prefix check scattered across callers. That is also what makes it testable at all: a realAddressis always canonically encoded by the host, so the string-level rule is only reachable as a pure function. The CRC16 checksum is intentionally not re-derived - the host already guarantees canonical encoding, so that branch is unreachable and would only cost gas.#570 -
deprecate_productleaked the product-name indexcreate_productrejects a name that is still mapped to a live product (#514), anddeprecate_productalready released the(category, oracle_key)slot. It did not release the name. So retiring a product burned its name permanently: the admin could never re-launch the same product line, and a name nobody can buy stayed reserved forever - which is exactly the "that name is taken" message users see for a product that no longer exists.Retirement now releases the name, but only when the entry still points at the product being retired. A stale mapping left by an earlier product must not be dropped just because a different product happens to share the name. The pool product count and the oracle-key slot keep their existing behaviour.
#571 - confidence bounds were unpinned
The
[1,100]check already existed onsubmit_data,submit_encrypted_data,batch_submit_dataandsubmit_data_batch, but onlysubmit_data's0and101boundaries were covered. Nothing pinned that the two batch paths and the encrypted path reject the same values, that a rejected reading leaves noDataPointsentry behind, or that a batch containing one bad reading reverts as a whole rather than persisting its good entries.#569 - one claim per policy was unpinned
The
PolicyClaimindex already gates every creation path, but nothing held it. The new tests cover the cases that could plausibly have been missed: a repeated id inside one batch, a claim filed byauto_process(the ordering that matters in production, since parametric policies are settled that way) blocking a latersubmit_claim,auto_processsettling an existing claim in place rather than minting a second record, a settled policy staying unclaimable, and a refused duplicate consuming neither a claim id nor a queue slot.Testing
39 new tests, all passing.
To confirm the coverage is load-bearing rather than decorative, each fix was mutated in turn and the suite re-run against the broken build:
PolicyClaimguard removedcargo test --workspace --no-fail-fastis unchanged againstmain- same 39 pre-existing failures, no new ones, +39 new passing tests:cargo clippy --workspace --all-targetsreports 0 errors and no warnings attributable to the changed or new code.Note on unrelated failures
The 39 failures above all pre-date this branch and are not touched by it. The
risk-poolandclaims-processorsuites in particular have a substantial backlog of failing tests onmain; that is tracked separately and left alone here.