feat(contracts): refresh adapter cache before deposit/withdraw pricing - #485
Conversation
Adds refresh(env: Env) to YieldAdapterInterface. vault.rs calls it ahead of the total_assets() read used for deposit/withdraw pricing, so a depositor's own transaction is priced against up-to-date yield rather than a stale cache. - BlendAdapter::refresh() wraps the existing accrue(), which remains a public, permissionless entry point in its own right. - DefindexAdapter::refresh() is a no-op, since it already prices total_assets() live on every call. - Test-only adapters (MockAdapter, TestAdapter) get matching stubs. - Adds CachedMockAdapter (vault test-only, in its own module to avoid a contractimpl name collision with MockAdapter) that mimics Blend's caching behavior, used to prove the refresh() call -- not just live pricing -- is what keeps deposit/withdraw pricing correct. Closes drydocs#418
|
@Diyaaa-12 is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
CI's resolved soroban-sdk build treats Self::accrue(env) as returning a must-use value, while the locally pinned version treats it as unit -- likely a version drift since no Cargo.lock is committed. Silencing unused_must_use on refresh() keeps the call correct and warning-free in both cases.
|
Good to see you back, @Diyaaa-12, glad everything's alright. |
collinsezedike
left a comment
There was a problem hiding this comment.
The refresh() mechanism itself is correctly implemented and fixes deposit-side pricing. But the comment above the withdraw() refresh call needs to be corrected before merge: it states "The withdrawer's actual USDC payout below is still computed live by the adapter (e.g. Blend inside submit()), independent of this cache." That's not accurate. withdraw() sizes the adapter redemption from ADPT_SH, a counter that only ever tracks deposited principal (BlendAdapter::deposit returns the raw USDC amount it was given, never bTokens or anything yield-bearing). That sized amount is what gets passed to Blend's submit() as the withdrawal request, not something Blend computes independently. A withdrawer today only ever receives their principal back, regardless of real yield accrued.
This isn't a flaw in the implementation here, the issue itself was written with this premise, so the PR correctly does what #418 described. But the comment can't stay as written since it asserts something false about the contract's behavior.
Update that comment to accurately describe the current limitation (withdrawal payout does not yet reflect live yield) rather than claiming it's already handled correctly elsewhere.
I've filed #486 to track the actual withdrawal-sizing fix separately, that one's a larger, distinct change and doesn't block this PR, which stands on its own for the deposit-side pricing fix once the comment is corrected.
The comment claimed withdraw() payouts are computed live by the adapter (e.g. Blend inside submit()), independent of the cache. That's not accurate: withdraw() sizes the adapter-side redemption from ADPT_SH, a counter that only ever tracks deposited principal, so payouts are currently capped at principal regardless of yield accrued. The actual withdrawal-sizing fix is tracked separately in drydocs#486. Also corrects the same false claim in CachedMockAdapter's test double and its accompanying test, clarifying that its live-pricing withdraw() is an intentional simplification to isolate what refresh() itself affects, not a model of BlendAdapter's current behavior.
|
Hi @collinsezedike, thanks for the review fixed the inaccurate withdraw comment in the latest commit (fix: correct inaccurate withdraw payout comment per review). It now correctly states that payouts are currently capped at principal since withdraw() sizes the adapter redemption from ADPT_SH, and links to #486 for the actual fix. Same correction applied to the CachedMockAdapter test double's comments, which had the same inaccurate claim. |
|
All checks are green now except PR Title, which looks like a transient CI glitch (failed in 3s, and the title matches the required feat(scope): ... format) could you re-run that job when you get a chance? |
collinsezedike
left a comment
There was a problem hiding this comment.
The comment is corrected accurately, and you also caught the same false claim in CachedMockAdapter's test double and fixed that too, which I hadn't flagged but should have. Good catch.
CI is green. This lands the deposit-side pricing fix cleanly, with the withdrawal-sizing gap now correctly documented and tracked separately in #486.
Merging now.
Summary
refresh(env: Env)toYieldAdapterInterface.vault.rscalls it ahead of thetotal_assets()read used for deposit/withdraw pricing, so a depositor's own transaction is priced against up-to-date yield rather than a stale cache.BlendAdapter::refresh()wraps the existingaccrue(), which remains a public, permissionless entry point in its own right.DefindexAdapter::refresh()is a no-op, since it already pricestotal_assets()live on every call.MockAdapter,TestAdapter) get matching stubs.CachedMockAdapter(vault test-only, in its own module to avoid acontractimplname collision withMockAdapter) that mimics Blend's caching behavior, used to prove therefresh()call — not just live pricing — is what keeps deposit/withdraw pricing correct.Test plan
cargo testpasses locally (60/60 tests across vault, blend-adapter, defindex-adapter, router)cargo clippy --all-targets -- -D warningspasses with no warningsrefresh()'s resource cost viaenv.cost_estimate().resources()DefindexAdapterlive-pricing tests unaffected (no-oprefresh()doesn't change behavior)pnpm lint && pnpm typecheck && pnpm testpass locallyNote:
scripts/deploy-testnet.sh/known-pools.tsupdate (fresh vault + adapter deploy, contractId swap) is not included here — happy to follow up with that once this is approved, or if a maintainer can run the testnet deploy.Closes #418