feat: add withdraw rate to one way vault#356
Conversation
WalkthroughThis update introduces a withdrawal fee mechanism to the OneWayVault contract, adding a fee parameter, calculation, and accounting logic. It modifies withdrawal and redemption functions to deduct fees, track accrued fees, and emit relevant events, with comprehensive tests validating the new behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Vault
participant FeeCollector
User->>Vault: withdraw(assets, receiver, owner)
Vault->>Vault: calculateWithdrawalFee(assets)
Vault->>Vault: determine sharesToBurn and sharesForRequest
Vault->>FeeCollector: accrue withdrawal fee
Vault->>User: emit WithdrawRequested(gross shares burned)
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (16)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
bekauz
left a comment
bekauz
left a comment
There was a problem hiding this comment.
mostly lgtm, just have some confusion about the distinction between shares to burn and shares that are to be accounted for
| revert("Deposit fee cannot exceed 100%"); | ||
| } | ||
|
|
||
| if (decodedConfig.withdrawRateBps > BASIS_POINTS) { |
There was a problem hiding this comment.
I know that here BASIS_POINTS means 10_000 but maybe we can clarify that in either the const naming or make it obvious in some other way?
There was a problem hiding this comment.
/*
* @dev Constant for basis point calculations (100% = 10000)
*/it's there on the const
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
solidity/src/vaults/OneWayVault.sol (1)
569-588: Verify the rounding mode choice for share calculation.The implementation correctly handles withdrawal fees in the
redeemfunction. However, at line 579,_convertToShares(netAssets, Math.Rounding.Ceil)rounds up when calculating shares for the request. This means users might receive slightly fewer assets than the exact net amount due to rounding.While this protects the protocol from rounding errors, it's worth documenting this behavior or considering if
Math.Rounding.Floorwould be more user-friendly while still maintaining protocol safety.Consider adding a comment explaining the rounding choice:
// Calculate shares for request based on net assets (what will be processed) + // Round up to ensure protocol doesn't lose value due to rounding errors uint256 sharesForRequest = _convertToShares(netAssets, Math.Rounding.Ceil);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
solidity/src/vaults/OneWayVault.sol(10 hunks)solidity/test/vaults/OneWayVault.t.sol(29 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-and-deploy
- GitHub Check: solidity contracts
- GitHub Check: setup-local-ic
🔇 Additional comments (4)
solidity/test/vaults/OneWayVault.t.sol (1)
615-619: LGTM! The test correctly verifies the WithdrawRequested event emission.The comment and test properly verify that the event emits the gross shares burned (
redeemShares), not the net shares stored in the withdrawal request. This aligns with the implementation and addresses past review feedback.solidity/src/vaults/OneWayVault.sol (3)
491-507: LGTM! Well-implemented withdrawal fee calculation.The function correctly:
- Uses basis points for fee calculation
- Rounds up to prevent dust losses
- Optimizes gas by returning 0 early when no fee is configured
- Follows the same pattern as
calculateDepositFeefor consistency
529-547: LGTM! Withdrawal fee logic correctly implemented.The implementation properly:
- Calculates fees on the gross withdrawal amount
- Burns shares for the full amount (ensuring users can't avoid fees)
- Stores net shares in the withdrawal request for cross-chain processing
- Accumulates fees for later distribution
This aligns perfectly with the PR objective of charging withdrawal fees while maintaining proper accounting.
597-626: LGTM! The refactored_withdrawfunction properly handles the dual share amounts.The implementation correctly:
- Uses descriptive parameter names (
sharesToBurnandsharesForRequest) as suggested in past reviews- Burns the full share amount from the user's balance
- Stores the net share amount in the withdrawal request
- Emits the gross shares burned in the
WithdrawRequestedeventThis provides clear separation between what the user pays (gross shares) and what they receive (net shares after fees).
2 participants
Adds a withdrawalFeeBps to the OneWayVault.
How it works is simple: every time someone withdraws, we burn all the shares but we only store the shares equivalent to the net assets after charging the fee.
Summary by CodeRabbit
New Features
withdrawRateBps) in vault configuration.Bug Fixes
Tests