feat: Sovryn Perimeter Fee display on withdraw and close flows - #1151
Open
tjcloa wants to merge 9 commits into
Open
feat: Sovryn Perimeter Fee display on withdraw and close flows#1151tjcloa wants to merge 9 commits into
tjcloa wants to merge 9 commits into
Conversation
Ports the perimeter-fee UI to the public repo, rebased onto current develop (base moved 4971f72 -> 6cd8343). Shows the fee row, tooltip, and net 'You will receive' amount on lending withdrawals, borrower exits, Zero collateral withdrawal/close, and the surplus-claim view. Display is gated purely on on-chain state and fails hidden: the row renders only when the controller quotes an active policy with a non-zero rate and fee. While the perimeter is deployed-but-disabled (its state until SIP-0094 executes and the Exchequer enables charging), every form renders exactly as it does today. No feature flag, no env var. All user-facing copy says 'Perimeter fee' (renamed from the working title during this port, tests updated to pin the new copy). Internal identifiers and the on-chain surface-id constants are unchanged — the ids are keccak hashes verified against the deployed consumer contracts. The Spanish locale remains the app-wide stub (falls back to English), unchanged by this change.
❌ Deploy Preview for sovryn-dapp failed. Why did it fail? →
|
🦋 Changeset detectedLatest commit: 90ab5c0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The frontend quotes policy by surface id, and Phase 1 is being re-cut with
renamed ids, so these literals move with the contracts. Left alone the app
would resolve no policy and render nothing at all -- a silent blank, not an
error, which is the failure mode this file exists to prevent.
The preimages change shape as well as content: Phase 1 derived an id as
keccak256("COLFEE:" + name), and the re-cut hashes the name alone, with the
namespace carried inside the name. SURFACE_ZERO_WITHDRAW_COLL joins the set,
which Phase 1 charged on-chain without the app ever quoting it.
The test now pins both halves -- each id against its preimage AND against the
literal 32 bytes. The preimage assertion alone cannot catch a rename that
rewrites the constant and its own expectation in one pass, which is how a stale
prefix survived a full sweep before being caught by comparing against the
contracts.
16 suites, 93 tests passing.
No component reads it — the Zero collateral-withdrawal fee comes from the contract's own preview function, which resolves the surface on chain. The constant exists so the test can pin it, which is what catches an id drifting between the contracts and this app. Without the note the next reader removes it as dead code and the canary goes with it.
Found by a sharp-edges review of the fee display API.
The perimeter fails open on chain: an unreachable controller, a reverting call
or a surface carrying no policy all resolve to no fee. The hooks flattened that
into { active: false, rateBps: 0 } -- the same value a deliberately-zero rate
produces -- and the row renders nothing when no fee is shown. So "we could not
find out" and "there is no fee" were the same pixels: absence. On the Zero
status panel it was worse than absence, because that one puts a number on it
and would present the gross surplus as the amount you receive.
Two ways it misled a user rather than a developer:
- Every consumer destructured { active, rateBps } and dropped loading. The hook
returns its inactive fallback until the quote arrives, so the form promised
no fee during the first fetch of every page view.
- The catch swallowed reverts, RPC failures and decode errors alike, and the
result is negatively cached for the TTL. One blip pinned "no fee" for thirty
seconds while the chain would still charge.
The quote type now carries `unknown`, separate from a real answer of zero -- an
unset controller pointer stays a real answer, because the contract charges
nothing by construction in that case. getExitFeeDisplay is the single decision,
returns charged/none/unknown, and folds loading in so it cannot be forgotten.
ExitFeeRow takes `unknown` as a required prop, so the caller has to say which of
the two this is, and renders a labelled row with an em dash rather than silence.
Pre-existing in Phase 1, not introduced by the re-cut. 96 tests, up 3, pinning
each of the three states. The linter caught the one consumer I had wired the
flags into without using them, which turned out to be the one that displays a
number.
Second own review cycle, checking what the first fix missed. CloseCreditLine still routed through isExitFeeShown, so an unreadable quote fell to the same branch as a genuinely uncharged surface and the collateral figure was presented as the gross with no fee mentioned. Closing a line of credit returns collateral through a charged surface, so that is the wrong number to show without saying it might be. Same treatment as the other three consumers: the display decision goes through getExitFeeDisplay, and the unknown case attaches the tooltip that says the rate could not be read rather than staying silent. This is why the second cycle exists. The first pass fixed the shared row and three consumers and looked complete; the fourth reached the same state by a different route and nothing failed.
Four consumers independently called isExitFeeShown with raw fields and reached the same wrong conclusion — that a rate nobody could read is a rate of zero. Fixing them one at a time fixes today's four; it does not stop the fifth. The predicate is now module-private. getExitFeeDisplay is the only exported decision, and it takes the whole quote, so the state that was being dropped cannot be dropped. A future consumer that tries the old shortcut gets a compile error instead of a review finding. Its tests now run through the exported decision, and cover the two states the old predicate could not express at all: unknown, and still loading.
Second adversarial pass, on the fix from the first. Attaching an "unavailable" tooltip to the two Zero views was not enough: both still rendered a number, and the number was the gross. The surplus and the closing collateral both leave through a charged surface, so the gross is precisely what does NOT arrive. A tooltip beside a confident figure loses to the figure. Both now show an em dash when the rate could not be read. Also corrected the opposite error, which the same pass found: an explicit zero gross was being reported as unknown, so "fee unavailable" would have appeared on add-collateral and borrow forms, where nothing leaves and no fee is possible. That is a real answer of none, and now says so. The controller-pointer cache keeps a bounded staleness window at the moment governance pins the controller -- up to the 30s TTL of showing no fee while the chain has begun charging. Documented in place rather than papered over: it exists once, and the release order already covers it, since the dapp ships before charging is enabled. Closing it properly needs block-based invalidation in the shared cache, which is a wider change than this window justifies.
Spec-to-code compliance found this, and it is a regression I introduced today. FRONTEND_EXIT_FEE_UI_SPEC §3 is explicit: on a quote revert or a missing getter, render nothing — "fail-hidden, never fail-wrong" — and it calls out the not-yet-deployed case by name, because exitFeeController() does not exist on mainnet until the activation SIPs execute. Today's unknown state treated that revert as "could not read the rate", so every lending, borrow and Zero form would have grown a "Perimeter fee —" row on a chain where no perimeter exists. Shipping the dapp ahead of activation is the plan of record, so this would have been the state on day one for every user. The two failures were never the same thing, and the controller pointer tells them apart. A missing or reverting getter, or a pointer of zero, is a protocol without the perimeter: nothing is charged, the forms look untouched, and that is a real answer. Only once the pointer resolves is the perimeter live — and a preview or quote that fails after that genuinely means the rate is unknown, which is the case the earlier fix was for. Zero gets the same split by reading its own exitFeeController() first, so "before the perimeter ships" and "the perimeter is up but the preview failed" stop being one revert. Both halves now hold: nothing appears before activation, and after it a failed read never reads as "no fee".
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
1 participant
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.
Sovryn Perimeter Fee — fee display on withdraw and close flows
Shows the perimeter fee (rate, amount, and net "You will receive") on lending
withdrawals, borrower exits, Zero collateral withdrawal/close, and the surplus-claim
view. Part of the Security Perimeter phase 1 release (SIP-0094).
Dormant until governance acts
Display is gated purely on on-chain state and fails hidden: a fee row renders only
when the deployed ExitFeeController quotes an active policy with a non-zero rate and
fee for that surface. Until SIP-0094 executes and charging is enabled, every form
renders exactly as it does on
developtoday — no feature flag, no env var, nothing toconfigure. Once charging is enabled on-chain, the display lights up on its own (quotes
are cached for 30 seconds).
Merge timing — opposite of the contract PRs, deliberately
The contract PRs (Sovryn-smart-contracts#580, zero-contracts#10) merge only after
the SIP executes. This PR should merge and deploy before charging is enabled: the
UI is provably inert until then, and having it live first guarantees users see the fee
the moment it exists. The one ordering that must not happen is enabling charging while
the dapp still lacks this UI — fees would apply without being displayed.
Notes for reviewers
during this port; tests pin the new copy). Internal identifiers and the on-chain
surface-id constants are unchanged — the ids are keccak hashes verified against the
deployed consumer contracts.
develop. The TrezorconnectSrcsanitizer hardening thattravelled with the original branch is not in this PR — it already landed on
developindependently (d96a4247).regression there.