-
Couldn't load subscription status.
- Fork 231
Deposit-only vault composer #1776
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| ) VaultComposerSync(_vault, _assetOFT, _shareOFT) {} | ||
|
|
||
| /** | ||
| * @notice Cross-chain redemptions are disabled for this vault |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @notice Cross-chain redemptions are disabled for this vault | |
| * @notice Redemptions are disabled for this vault |
| bytes32 /*_redeemer*/, | ||
| uint256 /*_shareAmount*/ | ||
| ) internal pure override returns (uint256 /*assetAmount*/) { | ||
| // Disable redemption to prevent cross-chain redemptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Disable redemption to prevent cross-chain redemptions | |
| /// @dev Disable all redemptions |
|
|
||
| /** | ||
| * @notice Cross-chain redemptions are disabled for this vault | ||
| * @dev Users should interact with the vault directly for redemption |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a comment about redeemAndSend reverting atomically, and lzCompose calls refunding to the original chain.
| @@ -0,0 +1,43 @@ | |||
| // SPDX-License-Identifier: UNLICENSED | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it have to be unlicensed? Should we make it MIT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TRileySchwarz thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be same as the regular vault composer
| /** | ||
| * @notice Creates a new cross-chain vault composer | ||
| * @dev Initializes the composer with vault and OFT contracts for omnichain operations | ||
| * @param _vault The vault contract implementing ERC4626 for deposit/redeem operations | ||
| * @param _assetOFT The OFT contract for cross-chain asset transfers | ||
| * @param _shareOFT The OFT contract for cross-chain share transfers | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also super nit.
I would stick to the VaultComposerSync NatSpec comment.
/**
* @notice Initializes the VaultComposerSyncDepositOnly contract with vault and OFT token addresses
* @param _vault The address of the ERC4626 vault contract
* @param _assetOFT The address of the asset OFT (Omnichain Fungible Token) contract
* @param _shareOFT The address of the share OFT contract (must be an adapter)
*
* Requirements:
* - Share token must be the vault itself
* - Asset token should match the vault's underlying asset (overridable behavior)
* - Share OFT must be an adapter (approvalRequired() returns true)
*/Again, I'm ok with this as is.
In this PR: