Fix rounding drift by reading adapter's live share balance - #645
Conversation
Fixes drydocs#566, drydocs#556: The vault now correctly tracks ADPT_SH by reading the adapter's live share balance instead of estimating decrements. Added total_shares() to the YieldAdapterInterface to facilitate this.
collinsezedike
left a comment
collinsezedike
left a comment
There was a problem hiding this comment.
The total_shares() reconciliation itself is correct and well-placed in both deposit() and withdraw() (called after the adapter's own deposit()/withdraw(), so it reads post-operation state, not stale state). Two things block this from compiling as-is, both confirmed locally, plus a formatting issue and a broken doc comment.
cargo clippy --all-targets -- -D warnings fails outright: deposit() now declares both total_adapter_shares (the old ADPT_SH storage read, line 197) and adapter_shares (the old AdapterClient::deposit() return value, line 237) without using either, since ADPT_SH is now set from AdapterClient::total_shares() instead of total_adapter_shares + adapter_shares. cargo fmt --all -- --check also fails on the two new ADPT_SH lines, and both the PR title and the commit message fail commitlint/semantic-PR-title (no type: prefix).
| env.storage() | ||
| .instance() | ||
| .set(&ADPT_SH, &(total_adapter_shares + adapter_shares)); | ||
| .set(&ADPT_SH, &AdapterClient::new(&env, &adapter_addr).total_shares()); |
There was a problem hiding this comment.
Now that ADPT_SH is set from AdapterClient::total_shares() here, total_adapter_shares (declared at line 197 from the old ADPT_SH storage read) and adapter_shares (declared at line 237 from AdapterClient::deposit()'s return value) are both unused. cargo clippy -- -D warnings fails on this, confirmed locally. deposit() still needs to call AdapterClient::deposit(&amount) for its side effect, but the return value can be discarded, and the storage read at line 197 can be removed outright.
| env.storage() | ||
| .instance() | ||
| .set(&ADPT_SH, &(total_adapter_shares - adapter_shares_to_burn)); | ||
| .set(&ADPT_SH, &AdapterClient::new(&env, &adapter_addr).total_shares()); |
There was a problem hiding this comment.
cargo fmt --all -- --check fails on this line and its counterpart in deposit() above; run cargo fmt before pushing.
| /// Refreshes the adapter's cached total_assets by attempting to accrue yield. | ||
| /// A failure to accrue (e.g., if the underlying pool reverts) must bubble up | ||
| /// rather than being swallowed, to prevent the vault from operating on stale | ||
| /// pricing. This adapter relies on the standard panic handler: bubbling by |
There was a problem hiding this comment.
This docstring reads as broken prose now: "This adapter relies on the standard panic handler: bubbling by discarding the error preserves this function's pre-existing fail-loud behaviour..." "bubbling" and "discarding" are opposites, and the sentence doesn't parse. Looks like the new first three lines were spliced onto the tail of the old docstring without removing the leftover fragment. Please rewrite this as one coherent comment.
3 participants
Fixes #566, #556: The vault now correctly tracks ADPT_SH by reading the adapter's live share balance instead of estimating decrements. Added total_shares() to the YieldAdapterInterface to facilitate this.