Conversation
Code Coverage Report for src/ files
|
skimaharvey
force-pushed
the
feature/osu-1378-interface-bug-multistrategy
branch
6 times, most recently
from
fc40f2a to
3ea4308
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
skimaharvey
force-pushed
the
feature/osu-1378-interface-bug-multistrategy
branch
from
3e74a76 to
f7bf88f
Compare
|
@codex review and make sure that the ABI with our MultistrategyVault will be the exact same as the original Vyper implementation https://github.com/yearn/yearn-vaults-v3/blob/master/contracts/VaultV3.vy (exceptions to the what is stipulated in the PR description) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
qGolem
left a comment
qGolem
left a comment
There was a problem hiding this comment.
split snake case into its own pr to make it easier to review
skimaharvey
force-pushed
the
feature/osu-1378-interface-bug-multistrategy
branch
from
f7bf88f to
e2d1943
Compare
skimaharvey
changed the base branch from
develop
to
feature/osu-1402-change-to-snake-case-on-msv
skimaharvey
force-pushed
the
feature/osu-1402-change-to-snake-case-on-msv
branch
from
efdd76a to
6d2011a
Compare
skimaharvey
force-pushed
the
feature/osu-1378-interface-bug-multistrategy
branch
from
e2d1943 to
dbe88b1
Compare
skimaharvey
force-pushed
the
feature/osu-1402-change-to-snake-case-on-msv
branch
3 times, most recently
from
a53e64c to
9fea78e
Compare
skimaharvey
force-pushed
the
feature/osu-1378-interface-bug-multistrategy
branch
from
dbe88b1 to
421b258
Compare
skimaharvey
force-pushed
the
feature/osu-1378-interface-bug-multistrategy
branch
from
f9bf25c to
14aded9
Compare
skimaharvey
force-pushed
the
feature/osu-1378-interface-bug-multistrategy
branch
2 times, most recently
from
5fd0104 to
f0d6558
Compare
skimaharvey
force-pushed
the
feature/osu-1378-interface-bug-multistrategy
branch
from
0ce4209 to
a4b3d52
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
Findings
MEDIUM-1: assess_share_of_unrealised_losses Breaks Vyper Compatibility
Files:
src/core/MultistrategyVault.sol(lines 1926-1933)src/core/interfaces/IMultistrategyVault.sol(line 406)
Description: PR 333 removes the 2-param Vyper-compatible wrapper that existed on develop, leaving only the 3-param version:
| Branch | Versions Available |
|---|---|
| develop | ✅ 2-param + 3-param overloads |
| PR 333 | ❌ Only 3-param (2-param removed) |
Vyper VaultV3.vy external API (2 params):
def assess_share_of_unrealised_losses(strategy: address, assets_needed: uint256) -> uint256:
current_debt: uint256 = self.strategies[strategy].current_debt # looks up internally
return self._assess_share_of_unrealised_losses(strategy, current_debt, assets_needed)develop branch Solidity (before PR 333) - had matching 2-param wrapper:
function assess_share_of_unrealised_losses(address strategy_, uint256 assetsNeeded_) external view returns (uint256) {
uint256 currentDebt = _strategies[strategy_].currentDebt; // looks up internally
return _assess_share_of_unrealised_losses(strategy_, currentDebt, assetsNeeded_);
}PR 333 removes this wrapper, leaving only:
function assess_share_of_unrealised_losses(address strategy, uint256 currentDebt, uint256 assetsNeeded)Impact:
- Breaks Vyper ABI compatibility - contradicts the PR's stated goal
- Breaks existing integrations expecting the 2-param signature
Risk: Medium - breaks existing integrations and contradicts the PR's stated goal of Vyper interface compatibility.
MEDIUM-2: Interface Gap for get_default_queue()
Files:
src/core/MultistrategyVault.sol(line 715)src/core/interfaces/IMultistrategyVault.sol
Description: The function defaultQueue() was removed and replaced with get_default_queue() which correctly matches the Vyper function name. However, get_default_queue() is not declared in the interface.
Vyper verification:
# VaultV3.vy has:
def get_default_queue() -> DynArray[address, MAX_QUEUE]:Risk:
- Breaking change for integrators using
defaultQueue()(was non-standard name) - Interface consumers cannot call
get_default_queue()throughIMultistrategyVault
LOW-1: Utility Functions Removed
File: src/core/MultistrategyLockedVault.sol
Description: Two helper functions removed:
getTransferableShares(address user)getRageQuitableShares(address user)
Users must now calculate these values from custodyInfo(user).lockedShares manually.
Risk: Convenience regression for integrators. NatSpec in the contract explains the manual calculation approach, but no external documentation was added.
LOW-2: NatSpec/Implementation Mismatch for maxLoss Default
File: src/core/MultistrategyLockedVault.sol (lines 363-380)
Description: The NatSpec for the 3-parameter withdraw function documents maxLoss = 10000 (100% loss accepted) but the implementation passes maxLoss = 0 (no loss accepted):
/**
* @notice Withdraws assets with default parameters (maxLoss = 10000, default queue)
*/
function withdraw(uint256 assets, address receiver, address owner) ... {
...
_redeem(msg.sender, receiver, owner, assets, shares, 0, new address[](0));
// ^-- actual value is 0
}Analysis: The code is correct - it follows the same pattern as the parent MultistrategyVault where withdraw uses maxLoss=0 and redeem uses maxLoss=10000. This matches ERC4626 semantics: withdraw(assets) specifies exact assets (no slippage tolerance), while redeem(shares) burns exact shares (accepts any resulting assets). The NatSpec is simply a typo.
Risk: Documentation error only. No fund loss possible - at worst, users experience unexpected reverts and must use the 5-parameter version with explicit maxLoss.
|
Thank you for the report. Med 1 -> good catch and fixed |
skimaharvey
force-pushed
the
feature/osu-1378-interface-bug-multistrategy
branch
from
a9fc165 to
7c7dec3
Compare
|
Let's merge or close this; it's been months. @qGolem @skimaharvey |
|
fine to merge for me |
3 participants
What does this PR introduce ?
This PR addresses critical interface compatibility issues between the Solidity implementation and the original Vyper vault contracts. The changes ensure 1:1 compatibility with the Vyper implementation while maintaining ERC4626 compliance.
Changes
1. Missing Withdraw/Redeem Function Overloads
Added missing function overloads that were not originally ported from Vyper:
Withdraw Functions && Redeem functions:
MaxWithdraw/MaxRedeem Functions:
Design Decision: The Vyper implementation includes additional 4-parameter overloads (with just maxLoss parameter). These were intentionally omitted due to Solidity contract size constraints. The 3 and 5 parameter versions provide equivalent functionality while keeping the contract size manageable.