feat: upgrade mainnet land contract to V4 #1692
Conversation
adjisb
force-pushed
the
feat/mainnet_land_upgrade_v4
branch
from
9e7beea to
7ec72de
Compare
|
""" WalkthroughA new deployment artifact for the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🪧 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 (
|
|
Closed the wrong PR by mistake, reopen it |
adjisb
force-pushed
the
feat/mainnet_land_upgrade_v4
branch
from
7ec72de to
6a4a0e1
Compare
![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)
packages/deploy/deployments/mainnet/Land_V3.json (1)
1232-1235: Missing storage layout details.
ThestorageLayoutsection is empty (storage: [],types: null). While the proxy itself doesn’t introduce new slots, embedding the implementation contract’s storage layout aids upgrade safety audits and tooling.Consider generating and including the storage layout JSON for the new logic contract.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/deploy/deployments/mainnet/Land_V3.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: format
- GitHub Check: analyze
- GitHub Check: deploy
- GitHub Check: coverage
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (2)
packages/deploy/deployments/mainnet/Land_V3.json (2)
1181-1185: Verify constructor arguments for V4 initialization.
Theargsarray encodes the proxy’s constructor, including a bytes payload for initialization:"args": [ "0x1fC6479Bdc7511C6803aFF2f477E0fD3171606E0", "0x46Ac83D74091Dd54B503D89fa92b5C788Bcd3886", "0x485cc9550000000000000000000000003845badade8e6dff049820680d1f14bd3903a5d0000000000000000000000000e19ae8f9b36ca43d12741288d0e311396140df6f" ]Ensure the third element correctly encodes the new
initialize(...)signature and parameters for the V4 logic contract. An incorrect selector or padding here will render the proxy unusable.
1186-1187: ValidatenumDeploymentscounter.
ThenumDeploymentsfield is set to3. If this is the fourth deployment for this contract, it may need to be incremented to4to maintain accurate deployment history in hardhat-deploy.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/deploy/deployments/mainnet/Land.json (2)
57-61: Remove trailing underscores in output parameter names.
The outputs ofadmin()andimplementation()are namedadmin_andimplementation_. This breaks tooling that expects unnamed returns or conventional names. Consider reverting to unnamed outputs or using cleaner identifiers.Also applies to: 83-87
314-515: Validate new event declarations.
An extended set of events (e.g.,Approval,ApprovalForAll,ContractRegistered, …,Transfer) has been added. Verify that each:
- Correctly indexes parameters per ERC-721 standards.
- Matches the on-chain event signatures and ordering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/deploy/deployments/mainnet/Land.json(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: deploy
- GitHub Check: analyze
- GitHub Check: coverage
- GitHub Check: lint
- GitHub Check: format
- GitHub Check: test
🔇 Additional comments (5)
packages/deploy/deployments/mainnet/Land.json (5)
23-35: VerifyBeaconUpgradedevent usage in proxy ABI.
TheBeaconUpgraded(address indexed beacon)event is typical of beacon proxies, not the OpenZeppelin transparent proxy. Confirm this event is actually emitted by your deployedLandproxy or remove it if it’s not part of the on-chain artifact.
129-312: Review bulk addition of custom errors.
You’ve added numerous custom errors (AlreadyMinted,ERC721IncorrectOwner, …,OperatorNotAllowed). Please ensure:
- Names adhere to your style guide and don’t collide with OpenZeppelin errors.
- Each error is actually thrown in the implementation to save bytecode (vs.
requirestrings).
517-1444: Verify added function signatures.
The ABI now lists many new functions (approve,batchTransferFrom,mintQuad,supportsInterface, etc.). Cross-check each against the upgraded contract source to confirm:
- Parameter order and
internalTypeare accurate.- State mutability flags (
view,payable, etc.) align with the implementation.
1501-1501: ConfirmnumDeploymentsincrement.
You’ve updatednumDeploymentsto4. Double-check that this correctly reflects the total mainnet deployments of theLandproxy.
1506-1506: Verify newimplementationaddress.
The proxy’s implementation address changed to0x7f2cAd7d2ad6FD2b6cE281d40ae5434AC776BDfB. Ensure this is the intended V4 contract and that the upgrade transaction targeted this address.
…gistry feat: add deploy metadata registry on mainnet
…e_royalty_manager Feat/mainnet land upgrade royalty manager
Description
https://internal-jira.atlassian.net/browse/LR-41
Summary by CodeRabbit