Open
Conversation
|
Forge code coverage:
|
There was a problem hiding this comment.
Pull request overview
Updates sub-vault asset redemption to respect each sub-vault’s OsToken LTV constraint, preventing LowLtv reverts when redeeming via osToken mint+redeem, and adds a regression test covering the scenario.
Changes:
- Cap per-sub-vault
redeemAssetsbyconvertToAssets(metaVaultShares) * ltvPercentbefore minting osToken shares. - Add a new test that configures a low sub-vault LTV and exercises
redeemSubVaultsAssetsthrough anEthOsTokenRedeemer.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
contracts/vaults/SubVaultsRegistry.sol |
Caps redemption amounts by sub-vault LTV headroom before osToken minting to avoid LowLtv reverts. |
test/SubVaultsRegistry.t.sol |
Adds a regression test for redeeming sub-vault assets under low LTV conditions (plus a helper harvest function). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…enalty > 0 blocks
…on due to rounding
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…to zero osToken shares
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
contracts/tokens/OsTokenRedeemer.sol:1
getExitQueueMissingAssetsno longer accounts for already-available assets held by the redeemer (the previous logic subtractedavailableAssetsvsmissingAssets). With the current implementation, callers may overestimate how much funding is actually required to cover the target. If the intended meaning of 'missing assets' has changed, consider renaming or documenting the new semantics; otherwise, reintroduce available-asset netting (and keep/restore a test that covers the 'available assets exceed missing' case).
// SPDX-License-Identifier: BUSL-1.1
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
contracts/tokens/OsTokenRedeemer.sol:1
getExitQueueMissingAssetsno longer accounts for the redeemer’s already-available assets (and also stops reading_unclaimedAssets). This changes the meaning from 'net additional assets required' to 'gross assets corresponding to missing tickets'. If external callers use this for funding decisions, they may over-provision. Either restore the netting logic (available assets minus unclaimed) or rename/document the function to reflect the new semantics.
// SPDX-License-Identifier: BUSL-1.1
test/gnosis/GnoErc20Vault.t.sol:1
- This assertion hard-codes the expected
Transferamount tosharesBefore(the sender’s full share balance). But the production code emits the delta actually consumed (i.e.,exitShares = sharesBefore - sharesAfter), which may be less than the full balance depending on how many shares are required to escrow the specifiedosTokenShares. To make the test robust, assert on the emitted amount equal to the actual balance delta (e.g., by recording logs and decoding theTransferevent), or compute the expected queued shares based on the same conversion logic used intransferOsTokenPositionToEscrow.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tsudmi
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
contracts/tokens/OsTokenRedeemer.sol:1
getExitQueueMissingAssetsno longer accounts for assets already available in the redeemer contract (the previous logic subtracted available/unclaimed assets). As written, it will report a positivemissingAssetseven ifaddress(this)already holds enough assets to cover the gap, which can cause unnecessary swaps/funding and break callers that rely on it returning the net missing amount. Consider restoring the “availableAssets vs missingAssets” offset (or explicitly renaming/documenting the function and updating all call sites if the semantic intentionally changed).
// SPDX-License-Identifier: BUSL-1.1
test/gnosis/GnoErc20Vault.t.sol:1
- This expectation hard-codes the
Transferamount tosharesBefore(entire balance). The production change emitsexitSharescomputed as the delta in_balances[msg.sender], which may differ fromsharesBeforeif only part of the position is queued/consumed or if internal rounding affects share consumption. To make the test robust to legitimate internal accounting changes, compute the expected event amount based on the pre/post balance delta (or assert that the emitted amount equalssharesBefore - sharesAfter).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
contracts/tokens/OsTokenRedeemer.sol:1
getExitQueueMissingAssetsno longer accounts for assets already available in the redeemer (previously:availableAssets = _getAssets(address(this)) - _unclaimedAssets). As a result, it can overestimate missing assets and drive unnecessary swaps/funding even when the contract already has enough free assets to cover the delta. Consider restoring the available-assets adjustment (or, if semantics intentionally changed, rename/document the function to reflect that it returns a gross requirement rather than net missing assets).
// SPDX-License-Identifier: BUSL-1.1
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
No description provided.