test(contract): add invariant coverage for fee_bps upper bound - #1410
Open
Ajibose wants to merge 3 commits into
Open
test(contract): add invariant coverage for fee_bps upper bound#1410Ajibose wants to merge 3 commits into
Ajibose wants to merge 3 commits into
Conversation
|
@Ajibose 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! 🚀 |
|
@Ajibose is attempting to deploy a commit to the Jaja's projects Team on Vercel. A member of the Team first needs to authorize it. |
…Node#1244) set_fee already enforces 0 <= fee_bps <= 10000 via validate_protocol_fee, but this invariant lacked explicit test coverage. Add unit tests verifying boundary values (0, 10000) succeed, values above 10000 panic and leave the previously stored fee untouched, plus a dedicated invariant test asserting the bound holds across the admin fee-update flow.
Rebasing Epta-Node#1244's PR onto current upstream main surfaced several pre-existing CI failures that also affect main's HEAD, unrelated to the fee_bps invariant work but blocking a green PR: - rustfmt drift: CI runs rustc 1.98.0, one point release ahead of what formatted these files; reformat lib.rs/test.rs under 1.98.0 so `cargo fmt --check` matches CI. - packages/sdk lint errors: unused imports in events-drift.test.ts, a dead `callCount` in health.test.ts, and any/require usage in write.test.ts (now scoped with the same eslint-disable convention used elsewhere in the package for legitimate mock reflection). - packages/sdk typecheck errors: client.ts was missing the scvU64/ scvSymbol helpers (present in generated/client.ts) used by prepareLikePostTx/prepareTipTx/preparePoolDepositTx; events/types.ts declared post_id as number on three interfaces while every other post_id in the codebase (including this same file) is bigint. - packages/sdk test failures, now unmasked by the typecheck fix: the write.test.ts stellar-base mock's `Account` was a bare jest.fn() with no implementation, so asserted _accountId never matched; and ConnectionHealthMonitor.scheduleCheck's initial-check jitter used `Math.random() * backoffMs` (default 1000ms) instead of a small interval-scaled jitter, so the "immediate" first health check could be delayed up to a full second — racing past health.test.ts's 500ms waitFor and, once one test timed out without reaching monitor.stop(), leaking a live real-timer loop that polluted call counts in every subsequent test in the file.
Ajibose
force-pushed
the
fix/set-fee-upper-bound-enforcement
branch
from
August 28, 2026 22:13
8d266d1 to
8b1679c
Compare
CI's clippy -D warnings failed on unnecessary_cast: author_posts.len() already returns u32, so the trailing `as u32` was a no-op that only newer clippy started flagging.
Contributor
|
Resolve the conflicts please |
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 #1244
Summary
set_fee(lib.rs:2715) is the admin entry point for updating the protocol fee. The issue reported that it did not clamp/rejectfee_bpsvalues above10000(100%), which would make fee computation (amount * fee_bps / 10_000) exceed the transferred amount and could brick tip/pool flows.Findings
On inspection,
set_feealready callsvalidate_protocol_fee(&env, fee_bps)(lib.rs:2720), which delegates tovalidate_u32_range(env, "fee_bps", fee_bps, 0, MAX_FEE_BPS)(validation.rs:159-161) withMAX_FEE_BPS = 10_000(validation.rs:11). This already rejects (panics on) anyfee_bps > 10_000and leaves the previously stored value untouched, since the panic occurs beforeenv.storage().instance().set(&FEE_BPS, &fee_bps)is reached.So the enforcement described in the issue is already present in the production code. What was missing was explicit test coverage proving the invariant
0 <= fee_bps <= 10_000holds, per the issue's acceptance criteria ("Invariant test added"). This PR adds that coverage.Files Changed
Modified:
packages/contracts/contracts/linkora-contracts/src/test.rs— unit tests forset_feeboundary/rejection behaviorpackages/contracts/contracts/linkora-contracts/src/tests/invariants.rs— dedicated invariant tests for the fee_bps boundNo changes were needed to
lib.rs, since the guard already existed there.Tests Added
In
src/test.rs:test_set_fee_max_boundary_valid— settingfee_bps = 10_000(the maximum, 100%) succeeds and is stored correctly.test_set_fee_rejects_value_above_max— settingfee_bps = 20_000panics with"fee_bps must be between 0 and 10000".test_set_fee_rejects_value_above_max_leaves_stored_fee_unchanged— after a valid fee is set, an out-of-range update panics and the previously stored fee is left untouched (no partial/corrupted state).In
src/tests/invariants.rs:test_invariant_fee_bps_bounded_at_boundaries— asserts both boundary values (0and10_000) are accepted and stored as-is.test_invariant_fee_bps_never_exceeds_max— asserts an admin misconfiguration (fee_bps = 20_000) is rejected outright (not clamped) and that the stored fee remains at its last valid value.How to Test
From
packages/contracts/contracts/linkora-contracts:All new tests pass. The full
cargo test --libsuite has 16 pre-existing failures in unrelatedtip_*tests (confirmed present onmainprior to this change, unaffected by this PR).CI Green-up (unrelated pre-existing breakage)
This branch was rebased onto the current
main(was 46 commits behind), which surfaced thatmain's HEAD is itself red on Unit Tests / Lint / JS-TS typecheck, unrelated to fee_bps. Fixed alongside this PR so it can merge green:rustc 1.98.0formats a few unrelated spots inlib.rs/test.rsdifferently than the version these files were last formatted with. Reformatted under 1.98.0 to match CI.packages/sdklint errors: unused imports inevents-drift.test.ts, a deadcallCountinhealth.test.ts, andany/require()usage inwrite.test.ts(scoped with the sameeslint-disableconvention already used elsewhere in the package for legitimate mock reflection).packages/sdktypecheck errors:client.tswas missing thescvU64/scvSymbolhelpers already present ingenerated/client.ts, used byprepareLikePostTx/prepareTipTx/preparePoolDepositTx.events/types.tsdeclaredpost_idasnumberon three event interfaces while every otherpost_idin the codebase (including elsewhere in this same file) isbigint.packages/sdktest failures, unmasked once typecheck was fixed (previously the JS/TS job never got far enough to runpnpm test):write.test.ts's@stellar/stellar-basemock hadAccount: jest.fn()with no implementation, so the asserted_accountIdwas never actually set on constructed instances.ConnectionHealthMonitor.scheduleCheck's initial-check jitter usedMath.random() * backoffMs(default 1000ms) instead of a small interval-scaled jitter, so the "immediate" first health check could be delayed up to a full second — racing pasthealth.test.ts's 500mswaitForand, once one test timed out before reachingmonitor.stop(), leaking a live real-timer loop that polluted call counts in every later test in the file.None of this is related to the fee_bps invariant fix itself — it's cleanup needed to get a green CI run on top of current main.