Repository navigation
chore: merge feedback into main - #359
luchobonatti wants to merge 540 commits into
Conversation
After the token's setup closes, token role grants, the manager revoke, the admin hand-over, Set the compliance contract and Set the identity verifier go through propose_change. Governance lists the token's pending changes with Apply and Cancel. Closes #305
Attest price asks for the proof hash and an optional proof URI and sends them as the oracle's NavInput. Cycle shows the latest proof, and reads use accepted_at. Closes #235
…nst-branch-contracts
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
feat(app): collect a proof reference when attesting
feat(app): run the share token's admin changes through its timelock
feat(app): run the vault's timelocked changes through the console
…ments feat(app): follow the renamed vault arguments
With a pinned tag, stellar-scaffold no longer sees the action's container as its local network, starts its own and redeploys, so the app is built against this branch's contracts and fails on the type errors the app stack fixes. Pin the image again once the app compiles against them.
feat(app): name the NAV pause and follow the oracle's bounds
The previous commit blamed the pin for the app build failure. That was wrong: the same tree without the pin fails the same way. The app does not compile against this branch's contracts yet, which the app stack fixes; the pin is unrelated.
…contracts ci: build the app against the contracts the branch deploys
docs: write the threat model
chore: run CI on protocol 30 and read its ledgers
Withdrawal cancels and claims explain 6068 and 6073 instead of the token's old 304, and the token pause copy says it stops holder transfers only. Closes #340
After the oracle's setup closes, Set the oracle limits, attester and guardian grants, the admin hand-over and the oracle upgrade go through propose_change. Governance lists the oracle's pending changes with Apply and Cancel. Closes #337
Claims and cancels now look up the holder's request right after the epoch, before they check whether the epoch is priced. A holder with no request in the epoch gets RequestNotFound, not an error about the epoch's price. Closes #363
The README said the script restores and extends all of a deployment. It covers each contract instance, its Wasm and each pending timelock change. Any other entry is restored by the next call that needs it, and that call's sender pays the restore fee. Closes #362
Subscribe, redeem, cancel and claim explain NotAllowlisted and Frozen, a frozen holder's requests show as held, Cancel follows the contract's cancel window, and a converted or surrendered request says where it went. Refs #304
Request, cancel, price, a claim the reserve cannot cover, fund and claim, on a seeded deployment. The treasury deploys the free reserve first and the redemption is sized above what is left, because a claim is refused only when the liquid reserve is below that claim. send takes a label and prints the transaction hash, so a run can be linked to an explorer.
List one testnet transaction per queue step, from the harness scenario, and the command that repeats it. Closes #368
`npm run test` imports only each client's `src/`, which the app's own client generator also writes, so its guard checks only that. The guard before `npm run typecheck` also requires each contract client's `dist/`, which the harness imports. Both errors name `npm run build:clients`. Refs #420
Add the console URL and one testnet transaction per privileged operation, with the signing account, signers and threshold. Say which operations were left out and why. Closes #370
chore(harness): set the minimum epoch duration at deploy
test(contracts): name the refusals added since the last pass
fix(contracts): small consistency fixes in the vault and oracle
chore(ci): generate the contract clients before the JS checks
Contract errors are resolved at runtime from the error cases the generated clients carry, so failures expose the error's name next to its code. Tests derive codes from each client's error table by name. Refs #415
Per-action copy moves to transactionCopy/ as data read by one describeStatus, and contract errors match by name. The modal tests are split by section. Fixes AdminTransferPending for the vault and oracle, and pause errors no longer show on oracle actions. Closes #415
refactor(app): split the transaction modal's copy and map errors by name
…tions docs: record the admin panel transactions
docs: record the redemption queue on testnet
fix(app): ledger-time expiry, sequential epoch reads, strict network config
| expires_at: input | ||
| .expires_at | ||
| .min(now.saturating_add(cfg.freshness_duration)), |
There was a problem hiding this comment.
Hey, there’s one interaction between the reporter’s expiry and the cooldown that I wanted to bring up.
For example, with a one-hour freshness window and a 60-second cooldown.
- At time
1000, a report is accepted with an expiry of1010. - At
1011, the report is stale. - A fresh report submitted then would be refused because the cooldown is still active.
- The next report can only be accepted at
1060.
This creates a period where the feed is stale and the cooldown prevents its replacement, even if the attester already has an updated report ready. Deposits and epoch pricing would wait during that gap.
One option could be to reject a report if it would expire before the cooldown ends. That would prevent this combination, although I can see how it might be too restrictive for some use cases.
Keeping the current flexibility could also make sense. In that case, it would help to document this interaction explicitly, so operators understand that the configured freshness window alone doesn’t prevent the gap. They also need to account for the reporter’s expiry and how much validity remains when the report is accepted, including any submission delays.
| if s.reconciled || s.deposits_open > 0 || s.redeems_open > 0 { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Hey, there’s one interaction between rounding reconciliation and zero-share refunds that I wanted to bring up.
From following these paths, it looks like a refund can wait for reconciliation while reconciliation waits for that same refund.
For example, all amounts below are in the smallest token units.
- The vault holds
11asset units, including Carol’s deposit of1. - Alice and Bob each redeem
5share units in the same epoch, priced at1.1asset units per share unit. - Pricing commits
floor(10 × 1.1) = 11asset units, but Alice and Bob each receivefloor(5 × 1.1) = 5. - Carol’s deposit rounds to zero shares, so she needs her
1asset unit refunded.
After both redemptions settle, the vault still holds that unit, but it remains marked as committed rounding dust. Carol’s refund fails because there is no free reserve. Reconciliation could release it, but the open deposit request prevents reconciliation from running.
Perhaps one way to address this would be to release the cash rounding remainder once all redemptions for that epoch have settled, independently of any outstanding deposit claims or refunds. At that point, the actual redemption payouts are known.
That would make the remaining unit available for Carol’s refund. The cash reconciliation would need to be tracked separately so completing the epoch doesn’t release the same remainder again.
rcostadresch
left a comment
rcostadresch
left a comment
There was a problem hiding this comment.
Great work on this round of fixes! The shared timelock, the per-epoch settlement, the wiring checks in finish_setup with health() and the oracle expiry and proof changes cover the findings really well, and it's good to see the Wasm upgrade tests running in CI too.
We also saw that some of the feedback was kept as a design decision and written up in the threat model and the contract docs, which makes the trade-offs easy to follow. My two notes are suggestions and nothing blocking. Before merging, please take a look at @fazzatti's comments on the oracle cooldown and the settlement reconciliation.
| /// The vault's clock with its wind-down ignored, so a `compliance` grant still | ||
| /// lands after the normal delay: without a holder no freeze can be lifted. | ||
| struct ComplianceGrantHost; | ||
|
|
||
| impl TimelockHost for ComplianceGrantHost { | ||
| fn clock(e: &Env) -> Result<Clock, Refusal> { | ||
| let mut clock = <ShareToken as TimelockHost>::clock(e)?; | ||
| clock.winding_down = false; | ||
| Ok(clock) | ||
| } | ||
| } |
There was a problem hiding this comment.
Not a bug since the threat model’s X6 already covers it, more of a thought for #238. Once a wind-down is active it can’t be cancelled, so a module that fails on burn would block every claim for good. Letting RemoveModule through a wind-down with the normal delay, the same way this compliance grant goes through, could keep that case recoverable.
| /// A report outlives the notice, so an epoch's cancellation window does | ||
| /// not reopen on its own while the notice runs. | ||
| pub freshness_covers_notice: bool, |
There was a problem hiding this comment.
Small nit, not blocking. The deploy guide covers min_epoch_duration, but a vault set up by hand could still close setup without it, leaving the manager as the only one who can seal. A flag here would make that easy to spot.
Summary
Merges
feedbackintomain: the reviewed work on the contracts, the harness, the app and the docs.GitHub closes an issue from
Closes #Nonly when a pull request merges into the default branch. The pull requests below merged intofeedback, so their issues are listed here, each with the pull request that did the work. All of them were closed by hand except #315, which this merge closes.Issues
Test plan
CI on this pull request builds and tests
feedback's head againstmain.