chore: introduced check for emitting event correctly#404
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWLSETH transfer logic now skips balance updates and returns false when converted shares equal zero; Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant WLSETH
participant Allowance
participant Balances
participant Events
Caller->>WLSETH: transferFrom(_from, _to, _value)
WLSETH->>WLSETH: compute valueToShares (underlying↔shares conversion)
alt valueToShares > 0
WLSETH->>Balances: decrement _from shares
WLSETH->>Balances: increment _to shares
WLSETH->>Allowance: if caller != _from, spend allowance
WLSETH->>Events: emit Transfer(_from, _to, underlyingAmount)
WLSETH-->>Caller: return true
else valueToShares == 0
WLSETH->>Events: emit Transfer(_from, _to, 0)
WLSETH-->>Caller: return true
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/test/WLSETH.1.t.sol (1)
956-982: Strengthen zero-share assertions to pin full no-op semanticsThese tests should also assert sender balance is unchanged, and
transferFromshould explicitly assert intended allowance behavior (unchanged vs decremented). Right now only recipient invariants are pinned.Also applies to: 984-1015
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/test/WLSETH.1.t.sol` around lines 956 - 982, In testTransferEmitsZeroWhenValueTooSmallForShares (and the similar transferFrom tests at 984-1015) capture and assert the sender's pre-operation balance and allowance and then assert they remain unchanged after the call: record uint256 beforeSenderBalance = wlseth.balanceOf(_guy) (and uint256 beforeAllowance = wlseth.allowance(_guy, spender) for transferFrom tests), perform the transfer/transferFrom that converts to 0 shares, then assert wlseth.balanceOf(_guy) == beforeSenderBalance and (for transferFrom) assert wlseth.allowance(_guy, spender) == beforeAllowance (or if the intended behavior is to decrement allowance even when 0 shares are moved, assert the expected decremented value instead); add these explicit assertions to both the transfer and transferFrom test cases to pin no-op semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/src/WLSETH.1.sol`:
- Around line 214-221: The transferFrom flow reduces spender allowance before
checking transfer amount (valueToShares) causing zero-value no-op transfers to
still consume allowance; update transferFrom (the code that adjusts allowance)
so allowance is only decremented when valueToShares > 0 (or restore the
allowance if valueToShares == 0) — i.e., perform the valueToShares check before
mutating allowance or conditionally apply the allowance reduction, leaving
BalanceOf, emit Transfer, and the valueToShares branch logic intact.
---
Nitpick comments:
In `@contracts/test/WLSETH.1.t.sol`:
- Around line 956-982: In testTransferEmitsZeroWhenValueTooSmallForShares (and
the similar transferFrom tests at 984-1015) capture and assert the sender's
pre-operation balance and allowance and then assert they remain unchanged after
the call: record uint256 beforeSenderBalance = wlseth.balanceOf(_guy) (and
uint256 beforeAllowance = wlseth.allowance(_guy, spender) for transferFrom
tests), perform the transfer/transferFrom that converts to 0 shares, then assert
wlseth.balanceOf(_guy) == beforeSenderBalance and (for transferFrom) assert
wlseth.allowance(_guy, spender) == beforeAllowance (or if the intended behavior
is to decrement allowance even when 0 shares are moved, assert the expected
decremented value instead); add these explicit assertions to both the transfer
and transferFrom test cases to pin no-op semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70c6516c-248e-4e1f-8904-44ac6281285b
📒 Files selected for processing (2)
contracts/src/WLSETH.1.solcontracts/test/WLSETH.1.t.sol
There was a problem hiding this comment.
Pull request overview
This PR updates WLSETHV1 transfer event emission behavior for cases where an underlying-value transfer rounds down to zero shares, and adds tests to validate Transfer event values across mint/burn/transfer/transferFrom under rebasing and rounding conditions.
Changes:
- Update
_transferto emitTransfer(..., 0)when the requested underlying amount converts to0shares (no balance changes). - Add new tests asserting
Transferevent values for mint/burn/transfer/transferFrom after a rebase and for “too-small-to-convert” transfers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
contracts/src/WLSETH.1.sol |
Adjusts _transfer event emission behavior for zero-share conversions. |
contracts/test/WLSETH.1.t.sol |
Adds tests for Transfer event correctness under rebasing/rounding edge cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Changes to gas cost
🧾 Summary (99% most significant diffs)
Full diff report 👇
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #404 +/- ##
=======================================
Coverage 97.61% 97.62%
=======================================
Files 79 79
Lines 2309 2315 +6
Branches 261 263 +2
=======================================
+ Hits 2254 2260 +6
Misses 54 54
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/src/WLSETH.1.sol`:
- Around line 218-227: The Transfer event in _transfer currently emits the
requested `_value` even though the actual moved amount is computed via
`valueToShares` and stored in `BalanceOf`; change the event to emit the actual
underlying value by converting `valueToShares` back using
`river.underlyingBalanceFromShares(valueToShares)` (same approach as in
`mint`/`burn`) so the emitted amount reflects the true underlying moved value;
update the success branch in `_transfer` to call `emit Transfer(_from, _to,
river.underlyingBalanceFromShares(valueToShares))` and keep the failure branch
emitting 0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab20bc1a-7ae1-494b-ab53-e8148aaff73a
📒 Files selected for processing (1)
contracts/src/WLSETH.1.sol
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 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 2 out of 2 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.
🧹 Nitpick comments (1)
contracts/test/WLSETH.1.t.sol (1)
986-1020: Add an allowance assertion for the zero-sharetransferFrombranch.At Line 1012 you already assert
Transfer(..., 0)and unchanged balances. Please also assert allowance remains unchanged to fully lock down this edge case.Suggested test hardening
vm.startPrank(_approved); vm.expectEmit(true, true, true, true); emit Transfer(_from, _recipient, 0); wlseth.transferFrom(_from, _recipient, 1); vm.stopPrank(); + assert(wlseth.allowance(_from, _approved) == 1); // Balances unchanged since 0 shares transferred assert(wlseth.balanceOf(_recipient) == 0); assert(wlseth.sharesOf(_recipient) == 0); assert(wlseth.sharesOf(_from) == 100 ether); assert(wlseth.balanceOf(_from) == 1000 ether);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/test/WLSETH.1.t.sol` around lines 986 - 1020, The test testTransferFromEmitsZeroWhenValueTooSmallForShares is missing an assertion that the spender's allowance is unchanged after a transferFrom that converts to 0 shares; after calling wlseth.transferFrom(_from, _recipient, 1) (inside the vm.startPrank(_approved) block) add an assertion that wlseth.allowance(_from, _approved) still equals the pre-transfer allowance (which was set via wlseth.approve(_approved, 1) earlier) to ensure allowance is not decremented when 0 shares are transferred.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contracts/test/WLSETH.1.t.sol`:
- Around line 986-1020: The test
testTransferFromEmitsZeroWhenValueTooSmallForShares is missing an assertion that
the spender's allowance is unchanged after a transferFrom that converts to 0
shares; after calling wlseth.transferFrom(_from, _recipient, 1) (inside the
vm.startPrank(_approved) block) add an assertion that wlseth.allowance(_from,
_approved) still equals the pre-transfer allowance (which was set via
wlseth.approve(_approved, 1) earlier) to ensure allowance is not decremented
when 0 shares are transferred.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f685ffc8-2f7c-426a-96c9-da49bf8da3b3
📒 Files selected for processing (2)
contracts/src/WLSETH.1.solcontracts/test/WLSETH.1.t.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/src/WLSETH.1.sol
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Prafful <46891804+iamsahu@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 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 2 out of 2 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 2 out of 2 changed files in this pull request and generated no new 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 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Prafful <46891804+iamsahu@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mischat-galaxy
left a comment
mischat-galaxy
left a comment
There was a problem hiding this comment.
i am happy with this now, thanks!
…' into bs-2754-on-pectra * origin/chore/merge-main-post-byov-audit: chore: fixed tests Accounting Changes rebased to remove-keys (BS-2865) (#395) chore: introduced check for emitting event correctly (#404) feat: re-enable `claimRedeemRequests` during Slashing Containment Mode (#400) fix: revert M-06 suggested changes across WLsETH (#399) Bump version inline with audit (#394) [L-01][Certora Audit] fix: Slashing containment mode still allows fresh validator funding (#393) fix(BS-2927) add nonReentrant modifier to `claimRedeemRequests` (#389) [M-06][ProdSec] non-zero value that converts to zero shares (BS-2923) (#383) [M-01] fix for Direct LsETH transfers to the wrapper strand underlying and break wLsETH supply accounting (BS-2925) (#380) [L-05] fix: IRiver.1.sol `setKeeper` not declared in IRiverV1 interface (BS-2924) (#382) fix: internal review finding M-04 + add check in test (#381) [H-01] fix: Denylisted users can bypass freeze controls by unwrapping wLsETH via burn (BS-2885) (#378) [M-03] fix for Inconsistent initiator deny check allows bypass via River redeem path (BS-2926) (#379) [I-01] fix: ` removeValidators ` silently modifies operator limit without emitting SetOperatorLimit event, creating divergence between on-chain state and off-chain monitoring (BS-2921) (#377) chore: fix deploy script issues and redeploy devHoodi (#370)
Description
This pull request introduces important fixes and improvements to the
WLSETHV1contract and its tests, primarily focusing on correct event emission and allowance handling during transfers, especially in the context of rebasing and edge cases where the underlying value does not map to shares.Core contract fixes and improvements:
Transfer and Allowance Handling:
transferandtransferFromfunctions now properly returntrueon success, andtransferFromis protected against reentrancy. Allowance checks and spending are corrected to ensure atomicity and proper error handling._spendAllowancemethod now requires the current allowance to be passed in, reducing redundant storage reads and improving clarity.Event Emission and Edge Case Handling:
_transferfunction now emits aTransferevent with the correct underlying value. If the value to shares conversion yields zero (i.e., the transfer amount is too small), aTransferevent with value0is emitted, and balances remain unchanged.Testing enhancements:
Transferevents always emit the correct underlying value after rebasing, formint,burn,transfer, andtransferFromoperations.Transferevent with value0is emitted and no balances or allowances are changed.These changes improve the correctness, safety, and transparency of the contract, especially in scenarios involving rebasing and small-value transfers.
Notice
Pull Request Type
Testing
Summary by CodeRabbit
Bug Fixes
Tests