-
Notifications
You must be signed in to change notification settings - Fork 39
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
revert: upgrade to Core v22 #2354
Conversation
WalkthroughThe changes in this pull request primarily involve updates to Docker image versions and modifications to configuration management functions within the Dashmate project. Key alterations include downgrading the Docker image version in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
packages/dashmate/src/status/scopes/platform.js (1)
315-319
: Consider moving this fix to a separate PRThis bug fix for platform activation timing calculation appears unrelated to reverting Core 22 upgrade changes. To maintain clear change history and facilitate easier rollbacks if needed, consider moving this fix to a separate PR focused on bug fixes.
packages/dashmate/src/listr/tasks/setup/local/configureCoreTaskFactory.js (1)
Line range hint
400-445
: Simplify EHF activation check using optional chainingUse optional chaining in the EHF activation check to avoid potential
TypeError
exceptions.Apply the following change:
- isEhfActivated = blockchainInfo.softforks && blockchainInfo.softforks.mn_rr - && blockchainInfo.softforks.mn_rr.active; + isEhfActivated = blockchainInfo.softforks?.mn_rr?.active;🧰 Tools
🪛 Biome (1.9.4)
[error] 424-425: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
packages/dashmate/templates/core/dash.conf.dot
is excluded by!**/*.dot
📒 Files selected for processing (4)
packages/dashmate/configs/defaults/getBaseConfigFactory.js
(1 hunks)packages/dashmate/configs/getConfigFileMigrationsFactory.js
(0 hunks)packages/dashmate/src/listr/tasks/setup/local/configureCoreTaskFactory.js
(7 hunks)packages/dashmate/src/status/scopes/platform.js
(2 hunks)
💤 Files with no reviewable changes (1)
- packages/dashmate/configs/getConfigFileMigrationsFactory.js
🧰 Additional context used
🪛 Biome (1.9.4)
packages/dashmate/src/listr/tasks/setup/local/configureCoreTaskFactory.js
[error] 365-366: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 424-425: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (7)
packages/dashmate/configs/defaults/getBaseConfigFactory.js (2)
56-56
: Version downgrade approved but testing is required.
The Docker image downgrade from '22.0.0-rc.1' to '21' aligns with the PR objective to revert Core 22 upgrade. However, since no testing has been performed according to the PR description, this change requires validation.
Let's verify the version consistency across related configurations:
✅ Verification successful
Version consistency verified across configurations
The version check confirms that:
- The new Core 21 version is correctly set in the base config
- No remaining references to Core 22 exist in the codebase
- Other version references are only in test fixtures with older versions (19.x and 20.x)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to Core 22 or version inconsistencies
# Search for any remaining Core 22 references
echo "Checking for Core 22 references..."
rg -i "22\.0\.0|core.*22" --type js
# Search for Docker image version references
echo "Checking Docker image versions..."
rg "dashpay/dashd:[\"']?\d+" --type js
Length of output: 773
56-56
: Verify compatibility with platform components.
Since this is a core version downgrade, we need to ensure compatibility with other platform components (dapi, drive) that are using dynamic versioning: dashpay/dapi:${dockerImageVersion}
and dashpay/drive:${dockerImageVersion}
.
Let's check the version compatibility:
✅ Verification successful
Let me gather more information about the version references and configuration.
Core version 21 is compatible with platform components
The version compatibility is maintained through dynamic versioning in the configuration. The code shows that platform components (DAPI and Drive) use dockerImageVersion
which is derived from the package version's major number. When Core is set to version 21, the platform components will automatically use version 21 images through dashpay/dapi:${dockerImageVersion}
and dashpay/drive:${dockerImageVersion}
.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify version compatibility between core and platform components
# Check platform component version references
echo "Checking platform component versions..."
rg "dashpay/(dapi|drive):\$\{.*\}" --type js -A 5
# Check package.json for version constraints
echo "Checking package.json version constraints..."
fd "package.json" --exec cat {} \; | jq -r 'select(.dependencies != null) | .dependencies'
Length of output: 5918
Script:
#!/bin/bash
# Check for platform component version references in configuration files
echo "Checking configuration files for version references..."
rg "image.*dashpay/(dashd|drive|dapi)" --type js -A 2 -B 2
# Check for docker version variables
echo "Checking for docker version variables..."
rg "dockerImageVersion" --type js -A 2 -B 2
Length of output: 13596
packages/dashmate/src/listr/tasks/setup/local/configureCoreTaskFactory.js (5)
5-5
: Import waitForNodesToHaveTheSameSporks
The addition of the import statement for waitForNodesToHaveTheSameSporks
is correct and necessary for spork synchronization.
96-98
: Verify DIP3 activation height
The DIP3 activation height has been updated to 1000
. Please verify that this is the intended value for the network configuration.
129-148
: Approved: Wallet creation task
The Create wallet
task is correctly implemented with appropriate parameters for wallet initialization.
151-158
: Approved: Generating masternode collateral
The calculation of the collateral amount and the invocation of generateToAddressTask
are correctly implemented.
280-282
: Approved: Waiting for spork synchronization
The task to wait for all nodes to have the same sporks ensures proper network synchronization.
Issue being fixed or feature implemented
Core 22 release is delayed so we are going to release 1.6 ASAP since it contains important bug fixes.
What was done?
How Has This Been Tested?
None
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation