fix(node): update builtin main chain #120
Conversation
saiintbrisson
requested review from
devwckd,
functor-flow and
steinerkelvin
as code owners
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change introduces a major terminology refactor across the permission system, replacing "grantor/grantee" with "delegator/recipient" throughout the codebase, documentation, and APIs. It also updates agent registration/deregistration logic, adds a new migration for permission storage, introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Runtime
participant PermissionPallet
participant AgentPallet
User->>Runtime: delegate_emission_permission(origin, recipient, ...)
Runtime->>PermissionPallet: delegate_emission_permission(delegator, recipient, ...)
PermissionPallet->>PermissionPallet: Store permission (delegator, recipient)
PermissionPallet-->>Runtime: Result
User->>Runtime: register_agent(origin, name, url, metadata)
Runtime->>AgentPallet: register(agent_key, name, url, metadata)
AgentPallet->>AgentPallet: Check agent_key, reserve fee
AgentPallet-->>Runtime: Result
User->>Runtime: deregister_agent(origin)
Runtime->>AgentPallet: deregister(agent_key)
AgentPallet->>AgentPallet: Remove agent, refund stake
AgentPallet-->>Runtime: Result
Possibly related PRs
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
Cargo.toml (1)
86-87:jsonrpseeswitched tofullfeature-set – reconsider scope
fullpulls in both server & client (plus websocket, http, macros, tracing).
If the workspace only needs the server side, restricting to:jsonrpsee = { version = "0.24", features = ["server", "macros"] }keeps compile times & binary size down.
docs/permission0.md (2)
93-93: Consider minor style improvement.The static analysis suggests using "can" instead of "is able to" for more concise writing.
-For now, only the SUDO key is able to delegate curator permissions. +For now, only the SUDO key can delegate curator permissions.
298-298: Add subjects to complete sentences.The static analysis correctly identifies incomplete sentences that could be improved for clarity.
-Can only be called by the delegator or root. +This can only be called by the delegator or root. -Can be called by the delegator, root, or enforcement controllers (with sufficient votes). +This can be called by the delegator, root, or enforcement controllers (with sufficient votes). -Can only be called by the delegator or root. +This can only be called by the delegator or root.Also applies to: 310-310, 334-334
| type Migrations = ( | ||
| pallet_governance::migrations::v5::Migration<Runtime, RocksDbWeight>, | ||
| pallet_emission0::migrations::v2::Migration<Runtime, RocksDbWeight>, | ||
| pallet_permission0::migrations::v4::Migration<Runtime, RocksDbWeight>, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify migration implementation follows best practices.
The addition of the v4 migration for pallet_permission0 is appropriate for the terminology changes. However, ensure this migration uses VersionedMigration as required by the coding standards.
Additionally, based on the retrieved learnings, ensure runtime upgrades are tested before committing these storage changes.
🏁 Script executed:
#!/bin/bash
# Description: Verify that the permission0 v4 migration uses VersionedMigration wrapper
# Expected: Find VersionedMigration usage in the migration module
ast-grep --pattern $'pub type Migration<T, W> = frame_support::migrations::VersionedMigration<
$$$,
$$$,
$$$,
$$$,
$$$
>'
# Also check the migration module structure
rg -A 10 "pub mod v4" pallets/permission0/src/migrations.rsLength of output: 542
Confirm VersionedMigration Usage & Add Migration Tests
The Migration<T, W> alias in pallets/permission0/src/migrations/v4.rs already wraps VersionedMigration<3, 4, MigrateToV6<T>, Pallet<T>, W> as required.
Next steps:
-
Add an end-to-end runtime upgrade test covering the v3→v4 storage migration with realistic data to validate the change before merging.
-
Suggested test location:
•pallets/permission0/src/migrations/v4/tests.rs(or in your existing runtime test suite)
🤖 Prompt for AI Agents
In runtime/src/lib.rs at line 89, the Migration type is correctly used but lacks
an end-to-end test for the v3 to v4 storage migration. Add a comprehensive
runtime upgrade test simulating realistic data to verify the migration works as
expected. Place this test in pallets/permission0/src/migrations/v4/tests.rs or
integrate it into the existing runtime test suite to ensure coverage before
merging.
| pub mod v4 { | ||
| use polkadot_sdk::{ | ||
| frame_support::{migrations::VersionedMigration, traits::UncheckedOnRuntimeUpgrade}, | ||
| sp_weights::Weight, | ||
| }; | ||
|
|
||
| use crate::{Config, Pallet}; | ||
|
|
||
| pub type Migration<T, W> = VersionedMigration<3, 4, MigrateToV6<T>, Pallet<T>, W>; | ||
| pub struct MigrateToV6<T>(core::marker::PhantomData<T>); | ||
|
|
||
| mod old_storage { | ||
| use codec::{Decode, Encode, MaxEncodedLen}; | ||
| use polkadot_sdk::{ | ||
| frame_support::Identity, frame_support_procedural::storage_alias, | ||
| polkadot_sdk_frame::prelude::BlockNumberFor, sp_runtime::BoundedVec, | ||
| }; | ||
| use scale_info::TypeInfo; | ||
|
|
||
| use crate::{ | ||
| permission::{EnforcementAuthority, PermissionDuration, PermissionScope}, | ||
| AccountIdOf, Config, Pallet, PermissionId, | ||
| }; | ||
|
|
||
| #[storage_alias] | ||
| pub type Permissions<T: Config> = | ||
| StorageMap<Pallet<T>, Identity, PermissionId, PermissionContract<T>>; | ||
|
|
||
| #[storage_alias] | ||
| pub type PermissionsByGrantor<T: Config> = StorageMap< | ||
| Pallet<T>, | ||
| Identity, | ||
| AccountIdOf<T>, | ||
| BoundedVec<PermissionId, <T as Config>::MaxTargetsPerPermission>, | ||
| >; | ||
|
|
||
| #[storage_alias] | ||
| pub type PermissionsByGrantee<T: Config> = StorageMap< | ||
| Pallet<T>, | ||
| Identity, | ||
| AccountIdOf<T>, | ||
| BoundedVec<PermissionId, <T as Config>::MaxTargetsPerPermission>, | ||
| >; | ||
|
|
||
| #[derive(Encode, Decode, TypeInfo, MaxEncodedLen)] | ||
| #[scale_info(skip_type_params(T))] | ||
| pub struct PermissionContract<T: crate::Config> { | ||
| pub grantor: T::AccountId, | ||
| pub grantee: T::AccountId, | ||
| pub scope: PermissionScope<T>, | ||
| pub duration: PermissionDuration<T>, | ||
| pub revocation: RevocationTerms<T>, | ||
| /// Enforcement authority that can toggle the permission | ||
| pub enforcement: EnforcementAuthority<T>, | ||
| /// Last execution block | ||
| pub last_execution: Option<BlockNumberFor<T>>, | ||
| /// Number of times the permission was executed | ||
| pub execution_count: u32, | ||
| /// Parent permission ID (None for root permissions) | ||
| pub parent: Option<PermissionId>, | ||
| pub created_at: BlockNumberFor<T>, | ||
| } | ||
|
|
||
| #[derive(Encode, Decode, TypeInfo, MaxEncodedLen)] | ||
| #[scale_info(skip_type_params(T))] | ||
| pub enum RevocationTerms<T: Config> { | ||
| /// Cannot be revoked | ||
| Irrevocable, | ||
| /// Can be revoked by the grantor at any time | ||
| RevocableByGrantor, | ||
| /// Can be revoked by third party arbiters | ||
| RevocableByArbiters { | ||
| accounts: BoundedVec<T::AccountId, T::MaxRevokersPerPermission>, | ||
| required_votes: u32, | ||
| }, | ||
| /// Time-based revocation | ||
| RevocableAfter(BlockNumberFor<T>), | ||
| } | ||
| } | ||
|
|
||
| impl<T: Config> UncheckedOnRuntimeUpgrade for MigrateToV6<T> { | ||
| fn on_runtime_upgrade() -> Weight { | ||
| let _ = old_storage::PermissionsByGrantor::<T>::clear(u32::MAX, None); | ||
| let _ = old_storage::PermissionsByGrantee::<T>::clear(u32::MAX, None); | ||
|
|
||
| for (id, contract) in old_storage::Permissions::<T>::iter() { | ||
| crate::Permissions::<T>::set( | ||
| id, | ||
| Some(crate::PermissionContract { | ||
| delegator: contract.grantor, | ||
| recipient: contract.grantee, | ||
| scope: contract.scope, | ||
| duration: contract.duration, | ||
| revocation: match contract.revocation { | ||
| old_storage::RevocationTerms::Irrevocable => { | ||
| crate::RevocationTerms::Irrevocable | ||
| } | ||
| old_storage::RevocationTerms::RevocableByGrantor => { | ||
| crate::RevocationTerms::RevocableByDelegator | ||
| } | ||
| old_storage::RevocationTerms::RevocableByArbiters { | ||
| accounts, | ||
| required_votes, | ||
| } => crate::RevocationTerms::RevocableByArbiters { | ||
| accounts, | ||
| required_votes, | ||
| }, | ||
| old_storage::RevocationTerms::RevocableAfter(block) => { | ||
| crate::RevocationTerms::RevocableAfter(block) | ||
| } | ||
| }, | ||
| enforcement: contract.enforcement, | ||
| last_execution: contract.last_execution, | ||
| execution_count: contract.execution_count, | ||
| parent: contract.parent, | ||
| created_at: contract.created_at, | ||
| }), | ||
| ); | ||
| } | ||
|
|
||
| Weight::zero() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix struct naming inconsistency.
The migration is upgrading from version 3 to 4, but the struct is named MigrateToV6. This should be MigrateToV4 for consistency.
-pub type Migration<T, W> = VersionedMigration<3, 4, MigrateToV6<T>, Pallet<T>, W>;
-pub struct MigrateToV6<T>(core::marker::PhantomData<T>);
+pub type Migration<T, W> = VersionedMigration<3, 4, MigrateToV4<T>, Pallet<T>, W>;
+pub struct MigrateToV4<T>(core::marker::PhantomData<T>);Also update the impl block:
-impl<T: Config> UncheckedOnRuntimeUpgrade for MigrateToV6<T> {
+impl<T: Config> UncheckedOnRuntimeUpgrade for MigrateToV4<T> {Verify storage map removal impact.
The migration clears PermissionsByGrantor and PermissionsByGrantee storage maps. Please ensure that no code relies on these indexed maps for querying permissions by delegator or recipient.
🏁 Script executed:
#!/bin/bash
# Search for any remaining usage of the removed storage maps
rg -A 3 "PermissionsByGrantor|PermissionsByGrantee|PermissionsByDelegator|PermissionsByRecipient" --type rustLength of output: 26781
Fix migration struct name to match migration version
The migration upgrades storage from version 3 to 4 but the struct and alias are named MigrateToV6. Rename them to MigrateToV4 to avoid confusion. We’ve verified that clearing old_storage::PermissionsByGrantor and PermissionsByGrantee has no unintended impact—no remaining code refers to those aliases outside the migration module.
• File: pallets/permission0/src/migrations.rs
• Rename MigrateToV6 → MigrateToV4
• Update the Migration alias and impl UncheckedOnRuntimeUpgrade
-pub type Migration<T, W> = VersionedMigration<3, 4, MigrateToV6<T>, Pallet<T>, W>;
-pub struct MigrateToV6<T>(core::marker::PhantomData<T>);
-impl<T: Config> UncheckedOnRuntimeUpgrade for MigrateToV6<T> {
+pub type Migration<T, W> = VersionedMigration<3, 4, MigrateToV4<T>, Pallet<T>, W>;
+pub struct MigrateToV4<T>(core::marker::PhantomData<T>);
+impl<T: Config> UncheckedOnRuntimeUpgrade for MigrateToV4<T> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub mod v4 { | |
| use polkadot_sdk::{ | |
| frame_support::{migrations::VersionedMigration, traits::UncheckedOnRuntimeUpgrade}, | |
| sp_weights::Weight, | |
| }; | |
| use crate::{Config, Pallet}; | |
| pub type Migration<T, W> = VersionedMigration<3, 4, MigrateToV6<T>, Pallet<T>, W>; | |
| pub struct MigrateToV6<T>(core::marker::PhantomData<T>); | |
| mod old_storage { | |
| use codec::{Decode, Encode, MaxEncodedLen}; | |
| use polkadot_sdk::{ | |
| frame_support::Identity, frame_support_procedural::storage_alias, | |
| polkadot_sdk_frame::prelude::BlockNumberFor, sp_runtime::BoundedVec, | |
| }; | |
| use scale_info::TypeInfo; | |
| use crate::{ | |
| permission::{EnforcementAuthority, PermissionDuration, PermissionScope}, | |
| AccountIdOf, Config, Pallet, PermissionId, | |
| }; | |
| #[storage_alias] | |
| pub type Permissions<T: Config> = | |
| StorageMap<Pallet<T>, Identity, PermissionId, PermissionContract<T>>; | |
| #[storage_alias] | |
| pub type PermissionsByGrantor<T: Config> = StorageMap< | |
| Pallet<T>, | |
| Identity, | |
| AccountIdOf<T>, | |
| BoundedVec<PermissionId, <T as Config>::MaxTargetsPerPermission>, | |
| >; | |
| #[storage_alias] | |
| pub type PermissionsByGrantee<T: Config> = StorageMap< | |
| Pallet<T>, | |
| Identity, | |
| AccountIdOf<T>, | |
| BoundedVec<PermissionId, <T as Config>::MaxTargetsPerPermission>, | |
| >; | |
| #[derive(Encode, Decode, TypeInfo, MaxEncodedLen)] | |
| #[scale_info(skip_type_params(T))] | |
| pub struct PermissionContract<T: crate::Config> { | |
| pub grantor: T::AccountId, | |
| pub grantee: T::AccountId, | |
| pub scope: PermissionScope<T>, | |
| pub duration: PermissionDuration<T>, | |
| pub revocation: RevocationTerms<T>, | |
| /// Enforcement authority that can toggle the permission | |
| pub enforcement: EnforcementAuthority<T>, | |
| /// Last execution block | |
| pub last_execution: Option<BlockNumberFor<T>>, | |
| /// Number of times the permission was executed | |
| pub execution_count: u32, | |
| /// Parent permission ID (None for root permissions) | |
| pub parent: Option<PermissionId>, | |
| pub created_at: BlockNumberFor<T>, | |
| } | |
| #[derive(Encode, Decode, TypeInfo, MaxEncodedLen)] | |
| #[scale_info(skip_type_params(T))] | |
| pub enum RevocationTerms<T: Config> { | |
| /// Cannot be revoked | |
| Irrevocable, | |
| /// Can be revoked by the grantor at any time | |
| RevocableByGrantor, | |
| /// Can be revoked by third party arbiters | |
| RevocableByArbiters { | |
| accounts: BoundedVec<T::AccountId, T::MaxRevokersPerPermission>, | |
| required_votes: u32, | |
| }, | |
| /// Time-based revocation | |
| RevocableAfter(BlockNumberFor<T>), | |
| } | |
| } | |
| impl<T: Config> UncheckedOnRuntimeUpgrade for MigrateToV6<T> { | |
| fn on_runtime_upgrade() -> Weight { | |
| let _ = old_storage::PermissionsByGrantor::<T>::clear(u32::MAX, None); | |
| let _ = old_storage::PermissionsByGrantee::<T>::clear(u32::MAX, None); | |
| for (id, contract) in old_storage::Permissions::<T>::iter() { | |
| crate::Permissions::<T>::set( | |
| id, | |
| Some(crate::PermissionContract { | |
| delegator: contract.grantor, | |
| recipient: contract.grantee, | |
| scope: contract.scope, | |
| duration: contract.duration, | |
| revocation: match contract.revocation { | |
| old_storage::RevocationTerms::Irrevocable => { | |
| crate::RevocationTerms::Irrevocable | |
| } | |
| old_storage::RevocationTerms::RevocableByGrantor => { | |
| crate::RevocationTerms::RevocableByDelegator | |
| } | |
| old_storage::RevocationTerms::RevocableByArbiters { | |
| accounts, | |
| required_votes, | |
| } => crate::RevocationTerms::RevocableByArbiters { | |
| accounts, | |
| required_votes, | |
| }, | |
| old_storage::RevocationTerms::RevocableAfter(block) => { | |
| crate::RevocationTerms::RevocableAfter(block) | |
| } | |
| }, | |
| enforcement: contract.enforcement, | |
| last_execution: contract.last_execution, | |
| execution_count: contract.execution_count, | |
| parent: contract.parent, | |
| created_at: contract.created_at, | |
| }), | |
| ); | |
| } | |
| Weight::zero() | |
| } | |
| } | |
| } | |
| pub mod v4 { | |
| use polkadot_sdk::{ | |
| frame_support::{migrations::VersionedMigration, traits::UncheckedOnRuntimeUpgrade}, | |
| sp_weights::Weight, | |
| }; | |
| use crate::{Config, Pallet}; | |
| pub type Migration<T, W> = VersionedMigration<3, 4, MigrateToV4<T>, Pallet<T>, W>; | |
| pub struct MigrateToV4<T>(core::marker::PhantomData<T>); | |
| mod old_storage { | |
| // … unchanged … | |
| } | |
| impl<T: Config> UncheckedOnRuntimeUpgrade for MigrateToV4<T> { | |
| fn on_runtime_upgrade() -> Weight { | |
| let _ = old_storage::PermissionsByGrantor::<T>::clear(u32::MAX, None); | |
| let _ = old_storage::PermissionsByGrantee::<T>::clear(u32::MAX, None); | |
| for (id, contract) in old_storage::Permissions::<T>::iter() { | |
| crate::Permissions::<T>::set( | |
| id, | |
| Some(crate::PermissionContract { | |
| delegator: contract.grantor, | |
| recipient: contract.grantee, | |
| scope: contract.scope, | |
| duration: contract.duration, | |
| revocation: match contract.revocation { | |
| old_storage::RevocationTerms::Irrevocable => { | |
| crate::RevocationTerms::Irrevocable | |
| } | |
| old_storage::RevocationTerms::RevocableByGrantor => { | |
| crate::RevocationTerms::RevocableByDelegator | |
| } | |
| old_storage::RevocationTerms::RevocableByArbiters { | |
| accounts, | |
| required_votes, | |
| } => crate::RevocationTerms::RevocableByArbiters { | |
| accounts, | |
| required_votes, | |
| }, | |
| old_storage::RevocationTerms::RevocableAfter(block) => { | |
| crate::RevocationTerms::RevocableAfter(block) | |
| } | |
| }, | |
| enforcement: contract.enforcement, | |
| last_execution: contract.last_execution, | |
| execution_count: contract.execution_count, | |
| parent: contract.parent, | |
| created_at: contract.created_at, | |
| }), | |
| ); | |
| } | |
| Weight::zero() | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In pallets/permission0/src/migrations.rs from lines 1 to 124, the migration
struct and alias are incorrectly named MigrateToV6 while the migration upgrades
storage from version 3 to 4. Rename the struct MigrateToV6 to MigrateToV4,
update the Migration type alias to use MigrateToV4 instead of MigrateToV6, and
change the impl block for UncheckedOnRuntimeUpgrade to implement for
MigrateToV4. This ensures the naming matches the migration version and avoids
confusion.
| // FIXME: Missing variables: | ||
| // CARGO_BIN_NAME, CARGO_BIN_EXE_<name> |
There was a problem hiding this comment.
Address FIXME comments in production code
Multiple FIXME comments indicate incomplete implementations:
- Lines 576-577: Missing CARGO_BIN_NAME and CARGO_BIN_EXE_ variables
- Lines 649-650: Commented-out implementation for CARGO_BIN_NAME
These should be either implemented or tracked in issues rather than left as comments in production code.
Would you like me to help implement these missing environment variables or create issues to track them?
Also applies to: 649-650
🤖 Prompt for AI Agents
In project-selector/src/main.rs around lines 576-577 and 649-650, there are
FIXME comments indicating missing implementations for the environment variables
CARGO_BIN_NAME and CARGO_BIN_EXE_<name>. To fix this, either implement the logic
to correctly set and use these variables in the code or create formal issues in
the project tracker to address these missing features later, then remove the
FIXME comments from the production code to keep it clean.
| proc_macro_dylib: Option<Utf8PathBuf>, | ||
| } | ||
|
|
||
| // TODO: wait for rust 1.90, https://github.com/rust-lang/cargo/pull/15674 |
There was a problem hiding this comment.
Remove TODO comment from production code
According to the codebase standards, TODO comments should not be left in production code. Either implement the feature or track it in an issue.
Would you like me to open an issue to track this Rust 1.90 feature requirement?
🤖 Prompt for AI Agents
In project-selector/src/main.rs at line 515, remove the TODO comment referencing
Rust 1.90 and the Cargo pull request, as TODO comments are not allowed in
production code. Instead, either implement the feature if possible or create and
reference a formal issue to track this requirement outside the codebase.
| if extension == std::env::consts::DLL_EXTENSION { | ||
| let name = path.file_stem()?.to_str()?.split_once('-')?.0.to_owned(); | ||
| let path = Utf8PathBuf::from_path_buf(path).ok()?; | ||
| eprintln!("{name} : {path}"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug print statement
This appears to be a debug print that should be removed from production code:
- eprintln!("{name} : {path}");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| eprintln!("{name} : {path}"); |
🤖 Prompt for AI Agents
In project-selector/src/main.rs at line 199, remove the debug print statement
eprintln!("{name} : {path}"); as it is not intended for production code and
should be eliminated to clean up the output.
|
|
||
| for line in out.lines() { | ||
| let message: Message = serde_json::from_str(line).unwrap(); | ||
|
|
There was a problem hiding this comment.
Replace unwrap() with proper error handling
Replace the unwrap() with proper error handling:
- let message: Message = serde_json::from_str(line).unwrap();
+ let message: Message = serde_json::from_str(line)
+ .context("Failed to parse cargo JSON message")?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - let message: Message = serde_json::from_str(line).unwrap(); | |
| + let message: Message = serde_json::from_str(line) | |
| + .context("Failed to parse cargo JSON message")?; |
🤖 Prompt for AI Agents
In project-selector/src/main.rs at line 531, the code uses unwrap() which can
cause the program to panic on error. Replace unwrap() with proper error handling
by matching on the Result or using methods like expect() with a clear error
message, or propagate the error using the ? operator to handle failures
gracefully.
saiintbrisson
force-pushed
the
fix/builtin-main-chain
branch
from
c4df2aa to
1c37c00
Compare
|
The built-in chain specfile for main and testnet have been out of date for a while. This patch updates it to ship with a new one containing the correct bootnodes.
Closes CHAIN-117