feat(vesting): release_all and single-call vesting overview; pin WASM-hash and release event audit trails (#409, #410, #408, #407) - #425
Merged
ritaifeoluwa merged 4 commits intoSep 27, 2026
Conversation
✅ Deploy Preview for sdcontracts ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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.
Implements #409, #410, #408, #407 — one commit per issue.
Read this first: three of these four issues report problems that were already fixed in
mainbefore this wave started.git log -Sconfirms it:mainset_pool_wasm_hashdoesn't emit the old hashlib.rs:1342already publishes(old_hash, new_hash)releasedoesn't emit an eventlib.rs:268already publishesvest/releasedget_vesting_schedulealready existsRather than skip them, each commit adds the missing guarantee the issue was really asking for — a regression test that pins the behaviour so it cannot silently regress, plus (for #409) the part of the request that genuinely was not there.
Also note the issue bodies cite
contracts/…; the workspace is atsoroban/contracts/….#410 — factory: WASM hash audit trail
The event already carries both hashes, but nothing tested it: a future edit to the payload could drop
old_hashand CI would stay green, which is precisely the "audit trail incomplete" outcome the issue describes.test_set_pool_wasm_hash_event_carries_old_and_new_hash— asserts the exactwasm_setevent, so the pair(old_hash, new_hash)is pinned.test_set_pool_wasm_hash_event_old_hash_matches_superseded_value— performs two consecutive changes and asserts the second event's old hash is the first change's new hash, i.e. the rollback chain is reconstructable from events alone.#408 — vesting: release event payload
The event exists, but the existing tests only asserted
!events.is_empty(), which passes even if every value in the payload is wrong. An indexer rebuilding vesting progress needs all three fields.test_release_event_payload_identifies_beneficiary_and_amounts— pins topic("vest","released")and the(beneficiary, amount_this_call, cumulative_total)payload.test_release_event_with_cumulative_total— two releases, asserting the second event reports 250 and a running 750.test_release_with_nothing_releasable_emits_no_event— a release with nothing vested must not emit, so an indexer can't record a release that never happened.I deliberately did not add
env.ledger().timestamp()as the issue's snippet suggests: this schedule is ledger-sequence based throughout (start_ledger,cliff_ledger,end_ledger, andcompute_vestedreadsenv.ledger().sequence()), so a wall-clock timestamp would be inconsistent with how the contract reasons about time. The cumulative released total is more useful to an indexer anyway.#409 — vesting: single-call schedule and progress
get_vesting_schedulereturns the configured parameters only. The issue's actual ask — start, end, cliff, total and released in one call — was still unmet: a frontend needed four round trips (get_vesting_schedule,vested_amount,released_amount,releasable), read at different ledgers.VestingOverview(types.rs) andget_vesting_overview()(lib.rs), returning the full schedule plusrevoked,vested_amount,released_amountandreleasable_amountatomically.VestingSchedule.VestingScheduleis a#[contracttype]already decoded by clients; adding fields would change its XDR and break them.VestingOverviewis additive and leaves the existing layout untouched.releasable_amountusessaturating_sub: afteremergency_withdrawzeroes the frozen vested amount whilereleasedis non-zero, a plain subtraction would hand callers a negative "releasable".read_start/read_end/… helper style in spirit by reusing the module's existingget_*helpers, which is what the contract already uses.Eight tests, including one asserting the combined call agrees with the three individual reads, and the revocation/emergency-withdraw saturation cases.
#407 — vesting: release_all
Implemented, but not with the issue's signature. The issue proposes
release_all(env, beneficiary)callingrelease(env, beneficiary, total); this contract'sreleasetakes nobeneficiaryargument — it reads the beneficiary from storage and requires that account's authorization specifically so third parties cannot force a release at a time the beneficiary did not choose. Passing a beneficiary in would either be ignored or reopen exactly the holerelease's doc comment warns about.pub fn release_all(env: Env) -> Result<i128, VestingError>delegates toSelf::release(env), so the arithmetic, the authorization and thevest/releasedevent stay on the single existing path — no second code path to drift.release()already transfers the whole vested-but-unclaimed balance in one transfer, so this is a self-documenting entry point rather than new machinery; the code says so explicitly instead of pretending otherwise.Nine tests: claims everything vested in one call, zero before the cliff, full amount after end, idempotent after a full claim, emits the same event as
release, requires beneficiary auth, rejects an unauthorized caller,NotInitializedon an uninitialized wallet, and counts as a release operation.Verification
Read-only verification per the task rules — no builds, installs or package-manager commands were run, so
cargo testhas not been executed. I compensated by verifying the arithmetic by hand againstcompute_vested(recomputing every expected vested/released value in the new tests with the same formula), and by matching the existing test idioms exactly:Events::all()tuple comparisons,try_*error matching,MockAuthInvokefor authorization, and theextern crate stdshim the factory and farming-pool tests use. New tests are collected by the existingcargo test.Closes #409
Closes #410
Closes #408
Closes #407