Skip to content
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

Optimize IpRoyaltyVaultStorage struct #100

Closed
Vectorized opened this issue Apr 15, 2024 · 5 comments
Closed

Optimize IpRoyaltyVaultStorage struct #100

Vectorized opened this issue Apr 15, 2024 · 5 comments

Comments

@Vectorized
Copy link

Vectorized commented Apr 15, 2024

struct IpRoyaltyVaultStorage {
    address ipId;
    uint32 unclaimedRoyaltyTokens;
    uint256 lastSnapshotTimestamp;
    mapping(address token => uint256 amount) ancestorsVaultAmount;
    mapping(address ancestorIpId => bool) isCollectedByAncestor;
    mapping(address token => uint256 amount) claimVaultAmount;
    mapping(uint256 snapshotId => mapping(address token => uint256 amount)) claimableAtSnapshot;
    mapping(uint256 snapshotId => uint32 tokenAmount) unclaimedAtSnapshot;
    mapping(uint256 snapshotId => mapping(address claimer => mapping(address token => bool))) isClaimedAtSnapshot;
    EnumerableSet.AddressSet tokens;
}

can be rewritten as:

struct IpRoyaltyVaultStorage {
    address ipId;
    uint32 unclaimedRoyaltyTokens;
    uint40 lastSnapshotTimestamp;
    mapping(address token => uint256 amount) ancestorsVaultAmount;
    mapping(address ancestorIpId => bool) isCollectedByAncestor;
    mapping(address token => uint256 amount) claimVaultAmount;
    mapping(uint256 snapshotId => mapping(address token => uint256 amount)) claimableAtSnapshot;
    mapping(uint256 snapshotId => uint32 tokenAmount) unclaimedAtSnapshot;
    mapping(uint256 snapshotId => mapping(address claimer => mapping(address token => bool))) isClaimedAtSnapshot;
    EnumerableSet.AddressSet tokens;
}

This saves a storage slot.

If you want to save 2 more slots for the majority of the case (e.g. tokens is small), I have add a drop-in replacement for EnumerableSet.AddressSet in Solady v0.0.191. It's under EnumerableSetLib.AddressSet. It has been heavily fuzz tested, but not audited.

@jdubpark
Copy link

We expect tokens size to be small, e.g. USDC, USDT, and WETH. Thanks for the alternative, please review cc @Spablob

For uint40 lastSnapshotTimestamp, any caution on casting between block.timestamp and uint40?

@Ramarti
Copy link

Ramarti commented Apr 15, 2024

We could also do condense the nested mappings into 1 level by hashing

mapping(uint256 snapshotId => mapping(address token => uint256 amount)) claimableAtSnapshot;

becomes

mapping(bytes32 key => uint256 amount) claimableAtSnapshot;

with the key being

keccack256(abi.encode(snapshotId, token, amount ))

@Spablob
Copy link

Spablob commented Apr 15, 2024

We expect tokens size to be small, e.g. USDC, USDT, and WETH. Thanks for the alternative, please review cc @Spablob

For uint40 lastSnapshotTimestamp, any caution on casting between block.timestamp and uint40?

Would keep it as ERC20 has - uint256. In future, we could be whitelisting a token that may need it?

@Spablob
Copy link

Spablob commented Apr 16, 2024

We expect tokens size to be small, e.g. USDC, USDT, and WETH. Thanks for the alternative, please review cc @Spablob

For uint40 lastSnapshotTimestamp, any caution on casting between block.timestamp and uint40?

agree with timestamp being uint40

@Spablob
Copy link

Spablob commented Apr 16, 2024

We could also do condense the nested mappings into 1 level by hashing

mapping(uint256 snapshotId => mapping(address token => uint256 amount)) claimableAtSnapshot;

becomes

mapping(bytes32 key => uint256 amount) claimableAtSnapshot;

with the key being

keccack256(abi.encode(snapshotId, token, amount ))

After further measurements by @ Ramarti we decided not to make this change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants