Test/transfer keys holder count - #842
Open
N-thnI wants to merge 2 commits into
Open
Conversation
creator-keys does not parse on main. The circuit-breaker check inside
buy_key's price branch was missing its closing brace:
} else {
let post_price = ...;
if pre_price > 0 && post_price > pre_price { // de-indented, unclosed
...
pre_price
}; // closes the if, not the else
so the brace intended for the else arm was consumed by the if, and the
unclosed impl CreatorKeysContract block ran to the end of the file:
"this file contains an unclosed delimiter" at lib.rs:7764.
The de-indentation makes the intent unambiguous -- pre_price is the else
arm's tail expression, not a value returned only when the breaker check
runs -- so the fix is the missing brace plus the indentation that was
presumably lost with it. No logic changed: the same comparison guards the
same early return, and the same pre_price is yielded.
This is a prerequisite rather than the issue's subject; nothing in the crate
compiles without it. See the accompanying test commit for what still blocks
a green build.
transfer_keys is the one path that can both remove and add a holder in a
single call, and holder_count is the number of wallets with a non-zero
balance. The existing transfer tests assert balances and supply but never
the holder count, so all three transitions were untested.
Six tests:
* full balance out, first-time recipient -- the sender leaves the holder
set as the recipient joins it. Net count is unchanged here, so the test
asserts both balances as well: a decrement-only or increment-only bug
would satisfy the total on its own.
* full balance out to an existing holder -- only the decrement can fire,
isolating it. Count 2 -> 1.
* partial transfer to a first-time recipient -- only the increment can
fire, isolating it. Count 1 -> 2.
* partial transfer between two existing holders -- neither side crosses
zero, so the count must not move.
* a round trip out and back -- a decrement not paired with its increment
would leave the count drifting across repeated transfers.
* repeat transfer to a wallet that already received keys -- it must not
be counted twice.
Not yet executed. creator-keys does not compile on main: the preceding
commit fixes an unclosed delimiter, and past that the crate reports 76
E0428 "defined multiple times" errors across events.rs (758-1253) and
lib.rs (362-5422) -- interleaved duplicate symbols, not one appended block,
consistent with merge 352f93a "Merge branch main into
feat/governance-quorum-requirement".
Resolving those means choosing which of two copies is authoritative for 76
symbols in a contract that moves funds, which is the merge author's call
and not something to guess at from here. The error count is identical with
and without these tests, so they introduce none; they should pass as
written once the duplicates are resolved.
Member
❌ CI Failed —
|
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 unit-test coverage for
transfer_keysholder-count accounting: a sender emptied out, a first-time recipient, and a partial transfer that changes neither.Closes #422
Important
These tests have not been executed.
creator-keysdoes not compile onmain. Details in "Blocked on" below — please read that section before merging.The implementation was already correct
transfer_keysalready handles all three transitions: it decrements when the sender's new balance reaches zero, increments when the recipient's prior balance was zero, and does neither on a partial transfer between existing holders. No production logic needed changing.What was missing is coverage.
test_transfer_keys_basic,test_transfer_keys_sender_zeroed_outandtest_transfer_keys_new_recipientall assert balances and total supply, but none of them readsholder_count— so every one of these transitions could regress without a test failing.Tests added (6)
full_balance_decrements_holder_countfull_balance_to_existing_holder_decrements_countfirst_time_recipient_increments_holder_countpartial_to_existing_holder_leaves_count_unchangedholder_count_survives_a_round_tripholder_count_never_exceeds_distinct_holdersWhy six rather than three. In the headline case — sender transfers their entire balance to a first-time recipient — the sender leaves the holder set as the recipient joins it, so the net count is unchanged at 1. A decrement-only bug and an increment-only bug would both satisfy a naive total-count assertion. The two isolating tests (full transfer to an existing holder; partial transfer to a new holder) each exercise exactly one branch, and the headline test asserts both balances alongside the count.
Blocked on:
creator-keysdoes not compile onmainTwo separate pre-existing problems, found in this order:
1. Unclosed delimiter — fixed here (
ca012db).The circuit-breaker check inside
buy_key's price branch was missing its closing brace:The brace intended for the
elsearm was consumed by theif, soimpl CreatorKeysContractran to end-of-file — reported asthis file contains an unclosed delimiteratlib.rs:7764. The de-indentation makes the intent unambiguous (pre_priceis the else arm's tail expression, not a value yielded only when the breaker check runs), so the fix is the missing brace plus the indentation presumably lost with it. Same comparison, same early return, same yielded value — no logic change.This is a prerequisite, not the issue's subject: nothing in the crate compiles without it.
2. 76 duplicate-definition errors — NOT fixed here.
Once parsing succeeds, the crate reports 76
E0428"defined multiple times" errors acrossevents.rs(lines 758–1253) andlib.rs(lines 362–5422) —FEE_COLLECTED_EVENT_NAME,LOCKUP_BLOCKED_EVENT_NAME,holder_cap_bps,last_buy_timestamp,credit_staking_rewards_pool,FeeCollectedEventand others.These are interleaved duplicate symbols spread across both files, not one cleanly appended block, which is consistent with merge
352f93a("Merge branch 'main' into feat/governance-quorum-requirement").I stopped rather than guess. Resolving this means deciding which of two copies is authoritative for 76 symbols — event names, storage keys, a rewards-pool credit function — in a contract that moves funds. Choosing wrong changes behaviour silently and would not show up as a compile error. That is the merge author's call, and it is well outside the scope of "add three unit tests."
Suggested path: raise the duplicate-symbol breakage as its own issue against the owner of
352f93a, and hold this PR until it lands. CI on this branch will be red until then, for reasons unrelated to this work.Verification status
ca012dbThe tests use only the existing public surface —
get_creator_holder_count,get_key_balance,buy_key,transfer_keys— and follow the setup pattern of the surrounding tests intest.rs, so they should pass as written once the duplicates are resolved. I would not merge on that assumption without a green run.Acceptance criteria
full_balance_to_existing_holder_decrements_count(isolated), plus the headline casefirst_time_recipient_increments_holder_countpartial_to_existing_holder_leaves_count_unchanged