mv poolBalances to regular mapping and array #708
Conversation
mv poolBalances to regular mapping and array
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
elshan-eth
requested review from
jubeira,
EndymionJkb and
joaobrunoah
and removed request for
jubeira
|
Maybe title this "Move..." We don't need to save bytecode in PR titles :) |
There was a problem hiding this comment.
Nicely done @elshan-eth, looks great! The optimizoooor badge is totally yours :)
I'll do one more pass after the remaining comments are addressed. I'd like to take a look at the existing tests to double check that we're not modifying functions that are lacking tests.
Co-authored-by: João Bruno - Balancer Labs <[email protected]>
Co-authored-by: EndymionJkb <[email protected]>
There was a problem hiding this comment.
Great job @elshan-eth !
I have a few comments, mostly minor, that I'd like to see addressed before proceeding.
Otherwise looks great; good gas savings and very sound idea! Thanks for pushing it :)
| @@ -75,7 +75,8 @@ contract VaultStorage { | |||
| // Pool -> (token -> PackedTokenBalance): structure containing the current raw and "last live" scaled balances. | |||
| // Last live balances are used for yield fee computation, and since these have rates applied, they are stored | |||
| // as scaled 18-decimal FP values. Each value takes up half the storage slot (i.e., 128 bits). | |||
| mapping(address => EnumerableMap.IERC20ToBytes32Map) internal _poolTokenBalances; | |||
| mapping(address => mapping(uint256 => bytes32)) internal _poolTokenBalances; | |||
There was a problem hiding this comment.
I'm thinking it'd make sense to use a custom type instead of bytes32 here (e.g. PackedBalances)
There was a problem hiding this comment.
I think we can do this in another PR because it seems like a big change for this PR. Is it ok?
|
|
||
| /// @dev Token not found in the token array. | ||
| error TokenNotFound(IERC20 token); | ||
| } |
There was a problem hiding this comment.
I'd reuse TokenNotRegistered (maybe adding the token argument)
Description
Type of change
Checklist:
main, or there's a description of how to mergeIssue Resolution