fix(remittance_nft): emit old/new value events from the two parameter setters (#1146)

[Banx17]

Closes #1146

What changed

set_default_burn_threshold and set_min_repayment_amount mutate protocol-critical instance config and published nothing, so off-chain consumers (indexer, webhooks, audit trails) could not observe changes to two of the highest-stakes risk parameters. Every other admin action in this contract (authorize_minter, revoke_minter, pause, unpause, admin transfer, set_loan_manager) already emits an event.

Both setters now capture the outgoing value before the overwrite and publish the full old -> new transition after the state write:

Setter	Topics	Data
set_default_burn_threshold	(Symbol::new(env, "DefaultBurnThresholdUpdated"), admin)	(old_threshold: u32, new_threshold: u32)
set_min_repayment_amount	(Symbol::new(env, "MinRepaymentUpdated"), admin)	(old_amount: i128, new_amount: i128)

Why this exact shape: contracts/loan_manager/src/events.rs documents the repo's admin config-update convention (("MinRepaymentUpdated", admin: Address) -> (old_amount: i128, new_amount: i128)), and backend/src/services/eventIndexer.ts already decodes that shape for every event in ADMIN_CONFIG_EVENT_TYPES (it reads topic[1] as the actor address and the second tuple element as the new value, e.g. // (type, admin), [old_amount, new_amount]). Reusing it means this event lands in the existing admin-config indexing/audit path with no indexer change at all, which is what the issue asked for ("so the indexer stays consistent"). Both names are longer than 9 chars, so Symbol::new is required (symbol_short! would not compile) - identical to the existing LoanMgrSet / AdminTransferred events in this same file.

Changed files

• contracts/remittance_nft/src/lib.rs - event emission in the two setters (+40 / -2)
• contracts/remittance_nft/src/test.rs - 1-line test rename + 4 new tests (+157 / -1)
• backend/** - CI-unblocking only: 4 files reformatted with prettier --write (f261f16f) and 3 test files repaired (2fc4fc7a). No production behaviour changes; details in caveat 7.

Done-when checklist from the issue

• set_default_burn_threshold emits an event with the old and new threshold -> test_set_default_burn_threshold_emits_old_and_new_value performs two real changes (3 -> 5, then 5 -> 9), asserting topics (DefaultBurnThresholdUpdated, admin) and data (old, new) for both events plus the persisted value. The second change proves the old value is read from storage, not hard-coded.
• set_min_repayment_amount emits an event with the old and new amount -> test_set_min_repayment_amount_emits_old_and_new_value does the same for 0 -> 1_000_000 then 1_000_000 -> 2_500_000 with an i128 payload.
• Tests assert the events publish with the expected topics/payloads -> the two tests above, plus test_set_default_burn_threshold_rejected_value_emits_no_event which pins that nothing is published when validation rejects the change (0 and MAX_ALLOWED_BURN_THRESHOLD + 1).
• cargo test, cargo fmt, clippy green; CI passes -> all 12 checks on this PR are green, including backend (Lint, Build, Type check, Run tests = 747 passed / 0 failed) and both CodeQL analyses. Getting there took two extra CI-unblocking commits for breakage that predates this branch; contracts itself is still only an echo (caveats 5 and 7), so the Rust command outputs below remain the real gate.

Raw encoded event, taken from the regenerated contracts/remittance_nft/test_snapshots/test/test_set_default_burn_threshold_upper_bound_valid.1.json (i.e. the existing test, which changed because it now emits):

[{"event":{"ext":"v0","contract_id":"0000...0002","type_":"contract","body":{"v0":{"topics":[{"symbol":"DefaultBurnThresholdUpdated"},{"address":"CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM"}],"data":{"vec":[{"u32":3},{"u32":1000}]}}}},"failed_call":false}]

topics = [DefaultBurnThresholdUpdated, admin], data = (old = 3 = DEFAULT_BURN_THRESHOLD, new = 1000).

Verification - exact commands and real output

Ran from contracts/ (where rust-toolchain.toml pins 1.85.0):

$ cargo test -p remittance_nft --lib
test result: ok. 92 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.56s
EXIT=0

88 of those are pre-existing tests (test_set_default_burn_threshold_upper_bound_valid, ..._invalid, test_record_default_auto_burns_after_threshold, test_transfer_rejects_burned_destination, ...) and they still pass unchanged - the event is purely additive. The 4 new ones:

test test::test_set_default_burn_threshold_emits_old_and_new_value ... ok
test test::test_set_default_burn_threshold_rejected_value_emits_no_event ... ok
test test::test_set_default_burn_threshold_upper_bound_invalid - should panic ... ok
test test::test_set_default_burn_threshold_upper_bound_valid ... ok
test test::test_set_min_repayment_amount_emits_old_and_new_value ... ok
test test::test_set_min_repayment_amount_requires_admin_auth - should panic ... ok

Baseline for comparison - the same command on a tree whose only difference from main is the test rename (no production change at all), which is what makes the "88 + 4 = 92" comparison meaningful:

test result: ok. 88 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.05s
EXIT=0

$ cargo fmt -p remittance_nft -- --check
(no output - clean)

$ cargo clippy -p remittance_nft --all-targets -- -D warnings
    Checking remittance_nft v0.0.1 (C:\Users\afeez\remitlend\contracts\remittance_nft)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 7.39s
EXIT=0

Caveats I hit - all pre-existing on main, none caused by this PR

1. contracts/remittance_nft/src/test.rs did not compile on main. It defined test_transfer_rejects_burned_destination() twice (lines 1230 and 1327), so cargo test for this crate failed with error[E0428]: the name ... is defined multiple times and none of its unit tests could run. Both definitions are real tests covering different paths (auto-burn via threshold + record_default vs. explicit burn()), so I kept both and renamed the first to test_transfer_rejects_auto_burned_destination (1-line change, separate commit f73237c9). This is required for the issue's own "tests assert the events publish" item to be verifiable at all, and it is why the baseline above is 88 tests. The contracts CI job does not compile anything today (see caveat 5), which is how this reached main; making that job real would be a worthwhile follow-up.

2. Plain cargo test -p remittance_nft cannot link in this Windows environment. Because the crate is crate-type = ["cdylib", "rlib"] and test builds pull in the testutils host dependencies, the test harness is linked as a dll and GNU ld aborts with ld: error: export ordinal too large: 68831. I verified this is environmental, not related to this change: on a tree whose only delta vs main is the one-line rename above, cargo test -p loan_manager --no-run fails exactly the same way for remittance_nft, lending_pool and loan_manager:

   $ cargo test -p loan_manager --no-run
   error: linking with `x86_64-w64-mingw32-gcc` failed: exit code: 1
     = note: ld: error: export ordinal too large: 68831
   error: could not compile `remittance_nft` (lib) due to 1 previous error
   EXIT=101
   

   cargo test -p remittance_nft --lib (what I ran) builds only the lib test harness and works fine, so the full unit suite really is exercised.

3. cargo fmt --all -- --check reports pre-existing drift in contracts/lending_pool/src/lib.rs:686,699 and contracts/loan_manager/src/lib.rs:1792, contracts/loan_manager/src/test.rs:5774 (line-wrap style plus a trailing newline). Those files are byte-identical to main in this branch (git diff upstream/main -- contracts/lending_pool contracts/loan_manager is empty), so this PR does not introduce it; cargo fmt -p remittance_nft -- --check is clean.

4. cargo clippy --workspace --all-targets -- -D warnings cannot compile loan_manager's test target on main either - pre-existing, in files untouched here:

   error[E0599]: no method named `get_total_due` found for struct `LoanManagerClient` --> loan_manager\src\test.rs:5601:29
   error[E0061]: this method takes 2 arguments but 3 arguments were supplied  --> loan_manager\src\test.rs:5677:13
   error[E0599]: no method named `get_total_due` found ...                    --> loan_manager\src\test.rs:5680:29
   

   The remittance_nft package itself is clippy-clean with -D warnings (output above).

5. The CI contracts job is a stub: .github/workflows/ci.yml only runs echo "Contracts format, clippy, tests, and build passed", so CI is green for this PR but could not have caught caveats 1 or 4. The Rust commands above are therefore the real gate.

6. Snapshot churn deliberately excluded. .gitignore line 5 lists contracts/**/test_snapshots/, but 46 snapshot files under contracts/remittance_nft/test_snapshots/ are nevertheless tracked (an ignore rule does not apply to already-tracked files), and because these tests capture snapshots via to_test_snapshot_file instead of asserting them, running the suite rewrites 7 of those tracked files. The same 7 files drift on main as well - the rename-only baseline rewrites the identical 7 with zero functional change - so none of that churn is in this diff, even though the event evidence above does come from one of those regenerated files.

7. The backend job was red for reasons unrelated to this PR, and is now fixed in it. Two independent pre-existing breakages were hiding behind each other, because the job aborts at the first failing step:
   a. Lint failed with 8 prettier/prettier errors (plus 30 warnings) in backend/src/services/auditLogService.ts, backend/src/services/__tests__/auditLogService.pagination.test.ts, backend/src/__tests__/remittanceFilters.test.ts and backend/src/tests/idempotency.namespace.test.ts. Those files are byte-identical to main on this branch and I reproduced the failure locally. Fixed in f261f16f by running prettier --write on exactly those four files (line-wrap and trailing-comma normalisation only). eslint . now reports 0 errors / 30 pre-existing warnings, so Lint, Build and Type check pass.
   b. Run tests then failed, because that step had never executed in CI: Tests: 9 failed, 33 skipped, 738 passed, 780 total. All 9 were test-side bugs - every implementation matches its own documented contract - and are fixed in 2fc4fc7a:

   • auditLogService.pagination.test.ts (5): asserted the keyset predicate while never passing a cursor; split the documented ${iso}:${id} cursor on the first : although ISO timestamps contain :; read the first SELECT of the test instead of the resumed one; compared the count SQL against a string missing the builder's trailing space; expected 8 filter keys where both AuditLogFilters and src/swagger/adminSwagger.ts document 7.
   • idempotency.test.ts (2): still expected the pre-[Security][Backend] idempotencyMiddleware uses raw un-namespaced Idempotency-Key allowing cross-user replay and denial-of-service #1809 un-namespaced idemp:<key> / idemp:<key>:lock.
   • idempotency.namespace.test.ts (2): the cache/lock mocks answered identically for every key, so Alice was handed Bob's cached body and hit Bob's lock; both mocks are now key-aware.
     Result on the same commit: Test Suites: 5 skipped, 101 passed, 101 of 106 total / Tests: 33 skipped, 747 passed, 780 total, and all 12 checks are green (backend in 1m19s, plus Analyze (rust) and Analyze (javascript-typescript)). Both commits are format/test-only, so nothing about [Contracts] RemittanceNFT parameter setters emit no events (set_default_burn_threshold, set_min_repayment_amount) #1146's behaviour changes - happy to split them out if you would rather review them separately.

Out of scope, intentionally left untouched

• No events added to migrate() / upgrade().
• No indexer-side parsing code touched - the only backend/ changes are the prettier reformat and the test repairs in f261f16f / 2fc4fc7a (caveat 7); no production logic changed.
• Neither setter's authorization or validation logic was changed. The threshold check still runs first, then require_auth(), then the pause check; Self::admin(&env) is only bound to a local so the same already-read address can be reused as the event actor (it was already evaluated before .require_auth() when the call was inlined). The negative-amount panic in set_min_repayment_amount is unchanged, and the new test_set_min_repayment_amount_requires_admin_auth test documents that admin-only enforcement still holds. Note that this setter previously had no test coverage of any kind.
• The emit cannot panic: it publishes a Symbol topic plus a fixed (u32, u32) / (i128, i128) data tuple, exactly like the existing AdminTransferred / MinScoreUpdated-style calls in this repo.

Commits

• c2f180c9 fix(remittance_nft): emit parameter-change events from the two config setters ([Contracts] RemittanceNFT parameter setters emit no events (set_default_burn_threshold, set_min_repayment_amount) #1146)
• f73237c9 fix(remittance_nft): rename duplicate test that breaks the test target
• cebaef82 test(remittance_nft): assert old/new values in the setter events ([Contracts] RemittanceNFT parameter setters emit no events (set_default_burn_threshold, set_min_repayment_amount) #1146)
• f261f16f style(backend): apply the prettier formatting the lint job requires
• 2fc4fc7a test(backend): repair the 9 stale tests that keep the backend job red