Columbus Deployment & Artifacts #60
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive refactoring of the cancellation module for the BookingToken system, specifically targeting the Columbus testnet. The changes include creating new deployment artifacts, introducing a new deployment module, and adding validation scripts. The refactoring involves updating smart contract implementations, proxy configurations, and adding new contract components to support enhanced cancellation functionality. Changes
Possibly related PRs
Suggested Reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ignition/modules/05_columbus_refactor_cancellation.js (1)
4-21: RefactorgetAddressesForNetworkinto a shared utility to avoid code duplicationThe function
getAddressesForNetwork(hre)is defined in multiple files, leading to code duplication. Consider extracting this function into a shared utility module to enhance maintainability and reduce redundancy.scripts/00_validate_01_force_import.js (2)
10-10: Clarify variable naming for consistencyThe variable
ContractV1represents theBookingTokenV2contract factory, which may cause confusion. Consider renaming the variable toBookingTokenV2for clarity and consistency.Apply this diff to rename the variable:
-const ContractV1 = await ethers.getContractFactory("BookingTokenV2"); +const BookingTokenV2 = await ethers.getContractFactory("BookingTokenV2");
25-25: Avoid unnecessary use ofprocess.exit(0)Using
process.exit(0)on successful completion is unnecessary, as the script will exit naturally. Additionally, explicit calls toprocess.exit()can prevent proper cleanup of asynchronous operations. Consider removing this call.Apply this diff to remove the unnecessary call:
-main() - .then(() => process.exit(0)) +main() + .then(() => { + // Success, no need to call process.exit(0) + })scripts/00_validate_01_BookingTokenRefactor.js (1)
45-94: Move extensive notes to separate documentationThe detailed notes and test outputs from lines 45 to 94 may clutter the test file and impact readability. Consider moving these notes to a separate documentation file or a dedicated section in the README to keep the test code clean and focused.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenImpl.dbg.json(1 hunks)ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenOperator.dbg.json(1 hunks)ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenOperator.json(1 hunks)ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenProxy.dbg.json(1 hunks)ignition/deployments/chain-501/artifacts/RefactorCancellationModule#CMAccount.dbg.json(1 hunks)ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.dbg.json(1 hunks)ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.json(1 hunks)ignition/deployments/chain-501/deployed_addresses.json(1 hunks)ignition/modules/05_columbus_refactor_cancellation.js(1 hunks)scripts/00_validate_01_BookingTokenRefactor.js(1 hunks)scripts/00_validate_01_force_import.js(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.dbg.json
- ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenProxy.dbg.json
- ignition/deployments/chain-501/artifacts/RefactorCancellationModule#CMAccount.dbg.json
- ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenOperator.dbg.json
- ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenImpl.dbg.json
🧰 Additional context used
🪛 Gitleaks (8.21.2)
ignition/deployments/chain-501/deployed_addresses.json
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
21-21: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (7)
ignition/modules/05_columbus_refactor_cancellation.js (1)
77-77: Verify if skippingreinitializeV2is appropriate for Columbus testnetThe
upgradeToAndCallfunction is called with an empty data parameter ("0x"), effectively skipping thereinitializeV2function call. Ensure that this is appropriate for the Columbus testnet and that omitting reinitialization will not lead to issues.scripts/00_validate_01_force_import.js (1)
13-17: Verify the proxy type matches the deployed contractEnsure that the proxy at address
0xe55E387F5474a012D1b048155E25ea78C7DBfBBCis indeed a UUPS proxy. If it's a Transparent Proxy, adjust thekindparameter accordingly to prevent deployment errors.scripts/00_validate_01_BookingTokenRefactor.js (1)
15-32: RefactorgetAddressesForNetworkinto a shared utility to avoid code duplicationThe function
getAddressesForNetwork(hre)is defined in multiple files, leading to code duplication. Consider extracting this function into a shared utility module to enhance maintainability and reduce redundancy.ignition/deployments/chain-501/deployed_addresses.json (1)
19-23: LGTM! The deployment addresses follow the expected proxy upgrade pattern.The address configuration shows:
- Proxy contracts (BookingTokenProxy, ManagerProxy) maintain their addresses for upgradeability
- New implementation contracts have unique addresses
- Clear separation between old and new module addresses
🧰 Tools
🪛 Gitleaks (8.21.2)
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
21-21: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenOperator.json (1)
6-84: LGTM! Robust error handling and payment type safety.The contract implements:
- Well-defined error types for payment validation
- Type-safe view functions for payment types
- Clear separation of concerns for payment handling
ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.json (2)
198-437: LGTM! Well-structured events with proper parameter indexing.The events are well-defined with:
- Proper indexing of critical parameters for efficient filtering
- Comprehensive coverage of state changes
- Clear naming conventions
877-913: Verify initialization parameters and access control setup.The initialize function accepts critical parameters that need careful validation:
- defaultAdmin
- pauser
- upgrader
- versioner
- developerWallet
- developerFeeBp
Run this script to verify the initialization call:
✅ Verification successful
Initialization parameters and access control setup verified successfully
The contract has proper validation and initialization of all critical parameters:
- Role assignments for defaultAdmin, pauser, upgrader, and versioner
- Developer wallet and fee basis points initialization
- Initial prefund amount setup (100 CAM)
- Zero address validation for critical parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify initialization parameters and role assignments # Search for initialize function calls rg -A 5 "initialize.*defaultAdmin.*pauser.*upgrader.*versioner.*developerWallet.*developerFeeBp" # Search for role assignments rg -A 3 "grantRole.*DEFAULT_ADMIN_ROLE|grantRole.*PAUSER_ROLE|grantRole.*UPGRADER_ROLE|grantRole.*VERSIONER_ROLE"Length of output: 113042
Script:
#!/bin/bash # Search for deployment scripts and tests related to initialization echo "=== Searching for deployment scripts ===" fd -e js -e ts "deploy.*Manager" -x cat {} echo -e "\n=== Searching for test files ===" fd -e js -e ts "test.*Manager" -x cat {} echo -e "\n=== Searching for validation patterns ===" rg -A 3 "assert.*address|require.*address|if.*address|revert.*address"Length of output: 116526
Script:
#!/bin/bash # Search for test cases related to initialization echo "=== Searching for initialization tests ===" rg -A 5 "describe.*initialize|it.*initialize" test/ echo -e "\n=== Searching for role validation tests ===" rg -A 5 "describe.*role|it.*role" test/ echo -e "\n=== Searching for storage tests ===" rg -A 5 "describe.*storage|it.*storage|it.*developer.*wallet|it.*prefund" test/Length of output: 14475
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
scripts/00_validate_01_BookingTokenRefactor.js (4)
1-12: Consider automating the setup process.The current setup requires manual git checkout of specific commits and running commands in sequence. This process could be error-prone and may break if the referenced commits become unavailable.
Consider creating a shell script to automate these steps:
#!/bin/bash # validate_storage_layout.sh set -e # Step 1: Force import network file git checkout a53b8fc03973025f4ed10edb46aa4d2f0c3e76ee yarn hardhat run scripts/00_validate_01_force_import.js --network columbus # Step 2: Run validation git checkout 1b5d14fca94a9129f517d8dd3e3d6eef47195d96 DEBUG=@openzeppelin:* yarn hardhat test scripts/00_validate_01_BookingTokenRefactor --network columbus # Return to original branch git checkout -
15-32: Enhance the network address utility function.The function is well-structured but could be more robust with a few improvements:
Consider these enhancements:
-function getAddressesForNetwork(hre) { +const SUPPORTED_NETWORKS = { + columbus: '../ignition/deployments/chain-501/deployed_addresses.json', + camino: '../ignition/deployments/chain-500/deployed_addresses.json', + localhost: '../ignition/deployments/chain-31337/deployed_addresses.json', +}; + +/** + * @param {import('hardhat/types').HardhatRuntimeEnvironment} hre + * @returns {Record<string, string>} Contract addresses for the current network + * @throws {Error} If the network is not supported + */ +function getAddressesForNetwork(hre) { let addresses; - if (hre.network.name === "columbus") { - console.log("Running on columbus"); - addresses = require("../ignition/deployments/chain-501/deployed_addresses.json"); - } else if (hre.network.name === "camino") { - console.log("Running on camino"); - addresses = require("../ignition/deployments/chain-500/deployed_addresses.json"); - } else if (hre.network.name === "localhost") { - console.log("Running on localhost"); - addresses = require("../ignition/deployments/chain-31337/deployed_addresses.json"); - } else { + const networkName = hre.network.name; + + if (!(networkName in SUPPORTED_NETWORKS)) { throw new Error(`Unsupported network: ${hre.network.name}`); } + + console.log(`Running on ${networkName}`); + addresses = require(SUPPORTED_NETWORKS[networkName]); return addresses; }
34-43: Enhance test coverage and error handling.The current test validates storage compatibility but could be more comprehensive.
Consider these improvements:
describe("Upgrade Check", function () { + let addresses; + let NewBookingToken; + + before(async function () { + addresses = getAddressesForNetwork(hre); + NewBookingToken = await ethers.getContractFactory("BookingToken"); + }); + it("Should validate storage compatibility", async function () { - const addresses = getAddressesForNetwork(hre); - - // Address at run time on Columbus: 0xe55E387F5474a012D1b048155E25ea78C7DBfBBC - const NewBookingToken = await ethers.getContractFactory("BookingToken"); - - await upgrades.validateUpgrade(addresses["CaminoMessengerModule#BookingTokenProxy"], NewBookingToken); + try { + await upgrades.validateUpgrade( + addresses["CaminoMessengerModule#BookingTokenProxy"], + NewBookingToken + ); + } catch (error) { + // Provide more context about the failure + console.error("Storage layout validation failed:"); + console.error(error.message); + throw error; + } }); + + it("Should verify proxy address exists", function () { + const proxyAddress = addresses["CaminoMessengerModule#BookingTokenProxy"]; + assert(ethers.utils.isAddress(proxyAddress), "Invalid proxy address"); + }); });
45-95: Improve documentation structure and visibility of breaking changes.The documentation of expected output is detailed but could be better structured to highlight important warnings about breaking changes.
Consider adding a prominent warning section at the top of the file:
/** * @notice BREAKING CHANGES * This upgrade includes intentional breaking changes to the BookingToken contract: * 1. Storage Layout Changes: * - Deleted: _cancellationProposals * - Modified: _isCancellable -> _proposals * - Modified: _bookingStatus enum values * * 2. ABI Changes: * These changes are accepted because: * - Contract is not deployed to mainnet * - Production usage is minimal * * @dev See detailed validation output below for specific incompatibilities */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/00_validate_01_BookingTokenRefactor.js(1 hunks)scripts/00_validate_01_force_import.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/00_validate_01_force_import.js
2a01d24 to
d0026e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ignition/modules/05_columbus_refactor_cancellation.js (2)
4-21: Consider a centralized configuration mechanism for network addresses.
The inlineif-elseapproach for different networks is functional but doesn't scale well if new networks need to be supported. Storing all network addresses in a single configuration file or using environment variables might simplify maintenance.
70-75: Document or remove commented-out code.
Leaving thereinitializeV2call commented out can cause confusion for future maintainers. Either provide a clear explanation (documenting why it remains disabled) or remove it if it's no longer needed.scripts/00_validate_01_BookingTokenRefactor.js (2)
15-32: Eliminate duplicated logic for retrieving addresses.
ThisgetAddressesForNetworkfunction is nearly identical to the one in05_columbus_refactor_cancellation.js. Consider extracting shared functionality into a common module or utility file to avoid duplication and reduce maintenance overhead.
34-43: Ensure thorough test coverage beyond storage layout.
While this test correctly validates storage compatibility, also consider testing functionality (e.g., verifying if cancellation or upgrade logic works as intended). Upgrades that pass storage checks can still introduce behavioral changes.Would you like me to outline additional test cases for functional verification?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenImpl.dbg.json(1 hunks)ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenOperator.dbg.json(1 hunks)ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenOperator.json(1 hunks)ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenProxy.dbg.json(1 hunks)ignition/deployments/chain-501/artifacts/RefactorCancellationModule#CMAccount.dbg.json(1 hunks)ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.dbg.json(1 hunks)ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.json(1 hunks)ignition/deployments/chain-501/deployed_addresses.json(1 hunks)ignition/modules/05_columbus_refactor_cancellation.js(1 hunks)scripts/00_validate_01_BookingTokenRefactor.js(1 hunks)scripts/00_validate_01_force_import.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.dbg.json
- ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenProxy.dbg.json
- ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenOperator.dbg.json
- ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenImpl.dbg.json
- ignition/deployments/chain-501/artifacts/RefactorCancellationModule#CMAccount.dbg.json
- scripts/00_validate_01_force_import.js
- ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenOperator.json
- ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.json
🧰 Additional context used
🪛 Gitleaks (8.21.2)
ignition/deployments/chain-501/deployed_addresses.json
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
21-21: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (2)
ignition/deployments/chain-501/deployed_addresses.json (2)
18-18: Double-check the updated CMAccount address.You’ve updated
CancellationModule#CMAccountto0xaA573b3942168bC5222C7aa11c6708D7f64Af374. If this is intentional, confirm that other references in the codebase whereCancellationModule#CMAccountis used have also been updated accordingly.To confirm, run:
✅ Verification successful
CMAccount address update is properly deployed and configured ✅
The new CMAccount address
0xaA573b3942168bC5222C7aa11c6708D7f64Af374was deployed through a proper deployment process and the ManagerProxy was updated to use this new implementation. No conflicting references were found in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search all references to CancellationModule#CMAccount and confirm usage consistency rg "CancellationModule#CMAccount"Length of output: 65847
19-23: Verify overlapping addresses and Gitleaks warnings.Several new entries for
RefactorCancellationModulereuse addresses already mapped elsewhere (e.g.,0xe55E387F...,0xE5B2f76C...). This may be intentional for internal upgrades or proxy reuse, but it could also lead to unexpected module conflicts. Additionally, Gitleaks flagged some addresses as “generic API keys.” Since these are standard blockchain addresses, it’s almost certainly a false positive. However, please verify that none of these addresses are actual secrets.Below is a script to check for duplicate addresses across modules:
✅ Verification successful
Address reuse is intentional and all formats are valid ✅
The address reuse appears to be intentional deployment strategy, likely for proxy contracts and shared implementations across chains (500 and 501). All addresses follow the standard Ethereum format (0x + 40 hex chars) and no suspicious patterns were found, confirming the Gitleaks warning as a false positive.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Identify overlapping addresses in deployed_addresses.json to verify intentional reuse. rg '"[^"]+#[^"]+": "[^"]+' deployed_addresses.json | sort | uniq -c | grep -v ' 1 'Length of output: 149
Script:
#!/bin/bash # First, find all deployed_addresses.json files echo "=== Searching for deployed_addresses.json files ===" fd "deployed_addresses.json$" echo -e "\n=== Checking for address overlaps across all files ===" # Extract all "address": "0x..." patterns, sort and count duplicates fd "deployed_addresses.json$" -x cat {} \; | grep -o '"0x[a-fA-F0-9]\{40\}"' | sort | uniq -c | grep -v "^ *1 " echo -e "\n=== Validating address format and checking for potential secrets ===" # Find any suspicious patterns that don't match standard Ethereum address format fd "deployed_addresses.json$" -x cat {} \; | grep -o '"0x[^"]*"' | grep -v '^"0x[a-fA-F0-9]\{40\}"$' || echo "No suspicious address formats found"Length of output: 1101
🧰 Tools
🪛 Gitleaks (8.21.2)
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
21-21: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
d0026e8 to
5325ce4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
ignition/deployments/chain-501/deployed_addresses.json (1)
19-20: LGTM! Proxy address reuse follows expected upgrade pattern.The reuse of proxy addresses (BookingTokenProxy and ManagerProxy) across modules is consistent with proper upgrade patterns, maintaining stable addresses for external integrations while allowing implementation updates.
Consider adding a comment in the JSON file or accompanying documentation to explicitly note this intentional address reuse pattern for future maintainers.
🧰 Tools
🪛 Gitleaks (8.21.2)
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
scripts/00_validate_01_BookingTokenRefactor.js (4)
1-12: Enhance documentation with additional context.Consider adding:
- Purpose of each git commit being checked out
- Expected output format
- Prerequisites for running the script
// Test for storage layout incompatibility +// Purpose: Validates storage compatibility between BookingToken contract versions +// Prerequisites: +// - Node.js and yarn installed +// - Access to the repository history +// // First force import the network file for the OZ's upgrades: +// Commit a53b8fc: Contains the original BookingToken contract // git checkout a53b8fc03973025f4ed10edb46aa4d2f0c3e76ee // yarn hardhat run scripts/00_validate_01_force_import.js --network columbus // // That will create a file ./openzeppelin/unknown-501.json // +// Then checkout the refactored version and run the test: +// Commit 1b5d14f: Contains the refactored BookingToken contract // git checkout 1b5d14fca94a9129f517d8dd3e3d6eef47195d96 // DEBUG=@openzeppelin:* yarn hardhat test scripts/00_validate_01_BookingTokenRefactor --network columbus
15-32: Consider enhancing error handling and caching.The function could benefit from:
- Caching the loaded addresses to avoid repeated file reads
- More descriptive error messages
+const addressCache = new Map(); + function getAddressesForNetwork(hre) { - let addresses; + const network = hre.network.name; + + if (addressCache.has(network)) { + return addressCache.get(network); + } - if (hre.network.name === "columbus") { + let addresses; + if (network === "columbus") { console.log("Running on columbus"); addresses = require("../ignition/deployments/chain-501/deployed_addresses.json"); - } else if (hre.network.name === "camino") { + } else if (network === "camino") { console.log("Running on camino"); addresses = require("../ignition/deployments/chain-500/deployed_addresses.json"); - } else if (hre.network.name === "localhost") { + } else if (network === "localhost") { console.log("Running on localhost"); addresses = require("../ignition/deployments/chain-31337/deployed_addresses.json"); } else { - throw new Error(`Unsupported network: ${hre.network.name}`); + throw new Error(`Unsupported network: ${network}. Supported networks: columbus, camino, localhost`); } + addressCache.set(network, addresses); return addresses; }
34-43: Expand test coverage for comprehensive validation.The current test only validates storage compatibility. Consider adding:
- Validation of function signatures
- Verification of events
- Testing of initialization values
describe("Upgrade Check", function () { + let NewBookingToken; + let addresses; + + before(async function () { + addresses = getAddressesForNetwork(hre); + NewBookingToken = await ethers.getContractFactory("BookingToken"); + }); + it("Should validate storage compatibility", async function () { - const addresses = getAddressesForNetwork(hre); + await upgrades.validateUpgrade(addresses["CaminoMessengerModule#BookingTokenProxy"], NewBookingToken); + }); - // Address at run time on Columbus: 0xe55E387F5474a012D1b048155E25ea78C7DBfBBC - const NewBookingToken = await ethers.getContractFactory("BookingToken"); + it("Should validate function signatures", async function () { + const oldArtifact = require("../artifacts/contracts/BookingTokenV2.sol/BookingTokenV2.json"); + const newArtifact = require("../artifacts/contracts/BookingToken.sol/BookingToken.json"); + + // Compare function signatures + const oldFunctions = new Set(oldArtifact.abi.filter(x => x.type === "function").map(x => x.signature)); + const newFunctions = new Set(newArtifact.abi.filter(x => x.type === "function").map(x => x.signature)); + + // Ensure no breaking changes in the public interface + for (const sig of oldFunctions) { + expect(newFunctions.has(sig), `Missing function: ${sig}`).to.be.true; + } + }); - await upgrades.validateUpgrade(addresses["CaminoMessengerModule#BookingTokenProxy"], NewBookingToken); + it("Should validate events", async function () { + const oldArtifact = require("../artifacts/contracts/BookingTokenV2.sol/BookingTokenV2.json"); + const newArtifact = require("../artifacts/contracts/BookingToken.sol/BookingToken.json"); + + // Compare events + const oldEvents = new Set(oldArtifact.abi.filter(x => x.type === "event").map(x => x.signature)); + const newEvents = new Set(newArtifact.abi.filter(x => x.type === "event").map(x => x.signature)); + + // Ensure no breaking changes in events + for (const sig of oldEvents) { + expect(newEvents.has(sig), `Missing event: ${sig}`).to.be.true; + } }); });
45-95: Structure the output documentation for better readability.Consider organizing the notes into clear sections with markdown formatting.
-// [Note@2025-01-23] -// -// Output for BookingTokenV2 contract that was used before the refactoring at -// https://github.com/chain4travel/camino-messenger-contracts/pull/55 +/** + * Expected Output Documentation + * Last Updated: 2025-01-23 + * + * Reference: BookingTokenV2 contract before refactoring + * PR: https://github.com/chain4travel/camino-messenger-contracts/pull/55 + * + * Expected Incompatibilities: + * 1. Storage Variables: + * - Deleted: `_cancellationProposals` + * - Note: Variable should be kept even if unused + * + * 2. Type Changes: + * - Location: contracts/booking-token/BookingTokenCancellable.sol:39 + * - Change: Replaced `_isCancellable` with `_proposals` (incompatible type) + * + * 3. Enum Changes: + * - Location: contracts/booking-token/BookingToken.sol:138 + * - Context: In mapping(uint256 => enum BookingToken.BookingStatus) + * - Changes: + * * Unspecified -> UNSPECIFIED + * * Reserved -> RESERVED + * * Expired -> RESERVATION_EXPIRED + * * Bought -> BOUGHT + * * Cancelled -> CANCELLED + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenImpl.dbg.json(1 hunks)ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenOperator.dbg.json(1 hunks)ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenOperator.json(1 hunks)ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenProxy.dbg.json(1 hunks)ignition/deployments/chain-501/artifacts/RefactorCancellationModule#CMAccount.dbg.json(1 hunks)ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.dbg.json(1 hunks)ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.json(1 hunks)ignition/deployments/chain-501/deployed_addresses.json(1 hunks)ignition/modules/05_columbus_refactor_cancellation.js(1 hunks)scripts/00_validate_01_BookingTokenRefactor.js(1 hunks)scripts/00_validate_01_force_import.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenProxy.dbg.json
- ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenOperator.dbg.json
- ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenImpl.dbg.json
- ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.dbg.json
- ignition/deployments/chain-501/artifacts/RefactorCancellationModule#CMAccount.dbg.json
- scripts/00_validate_01_force_import.js
- ignition/deployments/chain-501/artifacts/RefactorCancellationModule#BookingTokenOperator.json
- ignition/modules/05_columbus_refactor_cancellation.js
🧰 Additional context used
🪛 Gitleaks (8.21.2)
ignition/deployments/chain-501/deployed_addresses.json
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
21-21: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (3)
ignition/deployments/chain-501/deployed_addresses.json (2)
19-23: Ensure proper access controls for deployment artifacts.This file contains critical contract addresses that control system functionality. Please verify:
- The file is properly versioned and backed up
- Access to modify these addresses is strictly controlled
- A process exists to verify address integrity during deployments
Consider implementing a checksum verification system for these addresses to prevent tampering.
🧰 Tools
🪛 Gitleaks (8.21.2)
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
21-21: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
21-23: Verify deployment sequence and proxy configurations.Please ensure:
- The new BookingTokenImpl was properly initialized
- The proxy upgrade to the new implementation was executed correctly
- The new BookingTokenOperator has appropriate permissions
✅ Verification successful
Deployment sequence and proxy configurations are correct
The deployment module implements a proper upgrade sequence with:
- New BookingTokenOperator library deployment
- CMAccount implementation upgrade with the new operator
- BookingToken proxy upgrade with empty initialization (as noted for testnet)
The storage layout changes are intentional and documented for the testnet environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proxy configurations and permissions # Check if the BookingTokenProxy points to the new implementation cast storage 0xe55E387F5474a012D1b048155E25ea78C7DBfBBC 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc # Check if the BookingTokenOperator has required permissions cast call 0xe55E387F5474a012D1b048155E25ea78C7DBfBBC "hasRole(bytes32,address)(bool)" \ 0x0000000000000000000000000000000000000000000000000000000000000000 \ 0x9C4c78DbeaC83188Da9236cc3DfB06e86BE5d50cLength of output: 512
Script:
#!/bin/bash # Search for deployment scripts and configurations echo "=== Searching for deployment scripts ===" fd -e ts -e js deploy | grep -i "booking\|cancel" | xargs cat echo -e "\n=== Searching for proxy setup ===" rg -l "BookingTokenProxy|BookingTokenImpl|BookingTokenOperator" --type ts --type js echo -e "\n=== Searching for operator permissions ===" ast-grep --pattern 'grantRole($$$, $operator)'Length of output: 872
Script:
#!/bin/bash echo "=== Examining deployment module ===" cat ignition/modules/05_columbus_refactor_cancellation.js echo -e "\n=== Examining validation script ===" cat scripts/00_validate_01_BookingTokenRefactor.js echo -e "\n=== Checking for role assignments ===" rg -A 5 -B 5 "grantRole|DEFAULT_ADMIN_ROLE|OPERATOR_ROLE" ignition/modules/05_columbus_refactor_cancellation.jsLength of output: 7912
🧰 Tools
🪛 Gitleaks (8.21.2)
21-21: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.json (1)
1-1142: Verify the contract artifact's integrity.This is a machine-generated artifact file for the CMAccountManager contract. Ensure:
- The artifact matches the latest contract compilation
- The bytecode is deterministic
- The ABI includes all expected functions and events
✅ Verification successful
Contract artifact integrity verified successfully
The CMAccountManager contract artifact is complete and valid, containing:
- Valid bytecode and deployed bytecode
- All 44 required functions with proper signatures
- 14 events for state change notifications
- 21 error definitions for proper error handling
- Complete proxy and access control implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the contract artifact's integrity # Check if the artifact matches the latest contract compilation echo "Verifying contract compilation..." cd $(git rev-parse --show-toplevel) yarn hardhat compile diff ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.json artifacts/contracts/manager/CMAccountManager.sol/CMAccountManager.json # Verify all required functions are present in the ABI echo "Checking required functions in ABI..." required_functions=( "createCMAccount" "getAccountImplementation" "getAllRegisteredServiceHashes" "getAllRegisteredServiceNames" "getBookingTokenAddress" "getCMAccountCreator" "getDeveloperFeeBp" "getDeveloperWallet" ) for func in "${required_functions[@]}"; do if ! jq -e --arg func "$func" '.abi[] | select(.type=="function" and .name==$func)' ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.json > /dev/null; then echo "Missing required function: $func" exit 1 fi doneLength of output: 2646
Script:
#!/bin/bash # Verify contract artifact integrity # Check bytecode sections echo "Verifying bytecode sections..." jq -r '.bytecode' ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.json | grep -E '^0x[0-9a-f]+$' || echo "Invalid bytecode format" jq -r '.deployedBytecode' ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.json | grep -E '^0x[0-9a-f]+$' || echo "Invalid deployed bytecode format" # Verify key function signatures echo -e "\nVerifying function signatures..." jq -r '.abi[] | select(.type=="function") | "\(.name)(\(.inputs | map(.type) | join(",")))"' ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.json # Count events and errors echo -e "\nCounting ABI elements..." echo "Functions:" $(jq '[.abi[] | select(.type=="function")] | length' ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.json) echo "Events:" $(jq '[.abi[] | select(.type=="event")] | length' ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.json) echo "Errors:" $(jq '[.abi[] | select(.type=="error")] | length' ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.json) # Verify constructor echo -e "\nVerifying constructor..." jq '.abi[] | select(.type=="constructor")' ignition/deployments/chain-501/artifacts/RefactorCancellationModule#ManagerProxy.jsonLength of output: 52402
This PR adds deployment module and artifacts for Columbus.
Summary by CodeRabbit
New Features
Deployment
Infrastructure