feat(permission0): better instance handling #141
Conversation
saiintbrisson
requested review from
devwckd,
functor-flow and
steinerkelvin
as code owners
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
.helix/languages.toml (1)
5-6: Prefer rust-analyzer’s native flags over overrideCommandYou can avoid shelling out by using rust-analyzer options:
- check.allTargets = true (covers tests/examples)
- check.features = ["runtime-benchmarks"] (already added)
This is lighter and usually faster than overrideCommand.
check.extraEnv = { SKIP_WASM_BUILD = "true" } -check.features = ["runtime-benchmarks"] -check.overrideCommand = ["cargo", "check", "--message-format=json", "--tests", "--examples"] +check.features = ["runtime-benchmarks"] +check.allTargets = truepallets/permission0/src/permission/namespace.rs (2)
31-38: Fix typos in docs (“sibiling” → “sibling”)Minor polish to keep docs clear.
-/// This is true if enough instances are available and unused by -/// sibilings. +/// This is true if enough instances are available and unused by +/// siblings. @@ -/// Given a subpath, an instance is used if any parent or child -/// paths from that subpath were delegated to a sibiling permission. -/// Sibiling subpaths do not consume an instance relative to this +/// Given a subpath, an instance is used if any parent or child +/// paths from that subpath were delegated to a sibling permission. +/// Sibling subpaths do not consume an instance relative to this /// subpath.
39-44: Rename “other” to “paths” to avoid shadowing/confusionThe for-loop shadows the param; a clearer name reduces mistakes.
- pub(crate) fn check_available_instances<'a>( + pub(crate) fn check_available_instances<'a>( &self, this_id: PermissionId, - other: impl Iterator<Item = &'a NamespacePath>, + paths: impl Iterator<Item = &'a NamespacePath>, instances: u32, ) -> DispatchResult {pallets/permission0/src/ext/namespace_impl.rs (1)
256-256: Consider using iter().copied() to avoid &NamespacePath layeringTiny readability improvement; not required.
- parent.check_available_instances(parent_pid, new_paths.iter(), instances)?; + parent.check_available_instances(parent_pid, new_paths.iter(), instances)?;(If you adopt the rename in namespace.rs, keep param name in sync.)
pallets/permission0/tests/namespace.rs (3)
2156-2169: Prefer asserting specific error variants for critical failure paths.Several negative-path checks use
expect_err(...). Where stable (e.g., instance exhaustion), preferassert_err!(..., Error::<Test>::NotEnoughInstances)to lock in semantics and prevent regressions that still return Err but with the wrong reason.
2261-2261: Fix typo in failure message.“instace” → “instance”.
- delegate(bob, charlie, Some(parent), &[&h100], 1).expect_err("h100 instace pool depleted"); + delegate(bob, charlie, Some(parent), &[&h100], 1).expect_err("h100 instance pool depleted");
2128-2137: Use in-scope Bounded types and simplify deref.BoundedBTreeMap/BoundedBTreeSet are already imported at the top of this test file — replace crate::Bounded* with the in-scope names and change (**p).clone() → (*p).clone().
- let paths = TryCollect::<crate::BoundedBTreeMap<_, _, _>>::try_collect( + let paths = TryCollect::<BoundedBTreeMap<_, _, _>>::try_collect( vec![( parent, - TryCollect::<crate::BoundedBTreeSet<_, _>>::try_collect( - path.iter().map(|p| (**p).clone()), + TryCollect::<BoundedBTreeSet<_, _>>::try_collect( + path.iter().map(|p| (*p).clone()), ) .unwrap(), )] .into_iter(), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.helix/languages.toml(1 hunks)pallets/permission0/src/ext/namespace_impl.rs(3 hunks)pallets/permission0/src/permission/namespace.rs(2 hunks)pallets/permission0/tests/namespace.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-21T22:09:27.601Z
Learnt from: CR
PR: renlabs-dev/torus-substrate#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-21T22:09:27.601Z
Learning: Run cargo fmt, fix all clippy warnings, run checks/tests/coverage before committing
Applied to files:
.helix/languages.toml
🔇 Additional comments (2)
pallets/permission0/src/ext/namespace_impl.rs (1)
300-309: Revisit available_instances() vs new per-path capacity modelThis branch still uses parent.available_instances(), but capacity is now enforced via parent.check_available_instances in path resolution. Ensure available_instances() remains consistent with the new semantics or replace with the same utility for coherence.
I can scan the repo to locate available_instances() and confirm its behavior against the new rules.
pallets/permission0/tests/namespace.rs (1)
10-10: Import looks good.
DispatchErroris required for thedelegatehelper’sResult<_, DispatchError>.
saiintbrisson
force-pushed
the
dev
branch
3 times, most recently
from
3d19852 to
a768e6f
Compare
saiintbrisson
force-pushed
the
feat/permission0/better-instance-handling
branch
from
31e7527 to
f751152
Compare
saiintbrisson
force-pushed
the
feat/permission0/better-instance-handling
branch
from
f751152 to
8d99801
Compare
saiintbrisson
force-pushed
the
feat/permission0/better-instance-handling
branch
from
8d99801 to
d042eab
Compare
saiintbrisson
force-pushed
the
feat/permission0/better-instance-handling
branch
from
383a6d3 to
29fd31a
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
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 WalkthroughIntroduces instance-aware namespace delegation logic and a new runtime API to query available namespace instances. Updates benchmarks and autogenerated weights across multiple pallets, expands permission0 benchmarking scenarios and tests, and adjusts editor/lint settings. Several weight files are re-generated with new dates and metrics. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Runtime
participant Permission0 as Permission0 Pallet
participant Torus as Torus0 Pallet
rect rgb(240, 248, 255)
note over Client,Runtime: Runtime API call (new)
Client->>Runtime: available_namespace_instances(permission_id, path_inner)
Runtime->>Torus: validate/resolve NamespacePath from path_inner
Torus-->>Runtime: NamespacePath or error
Runtime->>Permission0: fetch permission by id
Permission0-->>Runtime: Permission (expect Namespace scope) or error
Runtime->>Permission0: scope.available_instances(permission_id, &NamespacePath)
Permission0-->>Runtime: remaining_instances (u32)
Runtime-->>Client: Ok(remaining_instances) or DispatchError
end
sequenceDiagram
autonumber
actor Caller
participant Perm as Permission0::namespace_impl
participant Scope as NamespaceScope
participant Parent as Parent Permission (optional)
participant Torus as Torus0
Caller->>Perm: delegate_namespace_permission_impl(...)
rect rgb(250, 250, 240)
Perm->>Perm: resolve_paths(new_paths, instances, parent?)
alt With parent
Perm->>Parent: get parent paths/capacity
Perm->>Torus: verify paths exist / relation checks
Perm->>Perm: check_namespace_delegation_depth(...)
Perm->>Parent: check_available_instances(parent_pid, new_paths, instances)
Parent-->>Perm: Ok or NotEnoughInstances
else Without parent
Perm->>Torus: verify delegator owns paths
end
Perm->>Perm: try_into bounded set (TooManyNamespaces on overflow)
end
Perm-->>Caller: proceed with delegation or error
note over Scope: Internally, available_instances()/check_available_instances\naccount for overlapping/hierarchical paths
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pallets/permission0/src/ext/namespace_impl.rs (1)
291-309: Bug: increasing child instances compares against parent.available_instances using absolute target, not delta.When raising a child’s max_instances, you should compare only the delta to the parent’s available instances. The current check can incorrectly fail legitimate increases.
Apply this diff:
- } else if max_instances > namespace.max_instances { - for parent in namespace.paths.keys().copied().flatten() { - let Some(parent) = Permissions::<T>::get(parent) else { - continue; - }; - - ensure!( - max_instances <= parent.available_instances().unwrap_or_default(), - Error::<T>::NotEnoughInstances - ); - } + } else if max_instances > namespace.max_instances { + // Only the increase (delta) must be within the parent's availability. + let delta = max_instances.saturating_sub(namespace.max_instances); + for parent in namespace.paths.keys().copied().flatten() { + let Some(parent) = Permissions::<T>::get(parent) else { + continue; + }; + ensure!( + delta <= parent.available_instances().unwrap_or_default(), + Error::<T>::NotEnoughInstances + ); + }
♻️ Duplicate comments (1)
pallets/permission0/src/ext/namespace_impl.rs (1)
219-243: Good fix: path-parent relation now checked per new_path (resolves prior bug).The validation now correctly ensures each new_path is either exactly present or a proper child of a parent_path (with existence check on the same agent). This addresses the earlier issue where unrelated descendants could pass.
🧹 Nitpick comments (11)
pallets/emission0/src/benchmarking.rs (1)
33-36: Good switch to saturating_mul on Balance.Safer arithmetic for deposit amount; multiline call improves readability.
Minor: Line 32 duplicates the allocator set already done on Line 22; consider dropping one to keep the benchmark lean.
justfile (1)
14-14: Tighten clippy to catch more and fail on warnings.Run across all targets and deny warnings for CI parity.
- cargo clippy --tests --features testnet,runtime-benchmarks + cargo clippy --all-targets --features testnet,runtime-benchmarks -- -D warnings.helix/languages.toml (1)
5-6: Unify check targets and avoid redundant feature flags.You can rely on
--all-targetsinstead of listing--tests/--examples, andcargo.featuresalready includesruntime-benchmarks, so duplicating incheck.featuresmay be unnecessary.-check.features = ["runtime-benchmarks"] -check.overrideCommand = ["cargo", "check", "--message-format=json", "--tests", "--examples"] +# check.features = ["runtime-benchmarks"] # Consider removing; `cargo.features` above already sets it +check.overrideCommand = ["cargo", "check", "--message-format=json", "--all-targets"]pallets/emission0/src/weights.rs (2)
97-101: Doc/const mismatch: min exec time comment vs Weight value.Comment says 9_000_000 ps but code uses 10_000_000. Align to avoid confusion.
- // Minimum execution time: 9_000_000 picoseconds. + // Minimum execution time: 10_000_000 picoseconds.
154-158: Same mismatch in the()impl.Keep comments consistent with
Weight::from_parts.- // Minimum execution time: 9_000_000 picoseconds. + // Minimum execution time: 10_000_000 picoseconds.pallets/torus0/src/weights.rs (1)
76-97: New read of Permission0::PermissionsByDelegator in stake paths—intentional?Adding this read to remove_stake increases DB access in a hot path. If this enforces wallet/permission gating, all good; otherwise consider gating behind a fast check or caching to avoid regressions.
Can you confirm this storage access is required for correctness in these extrinsics? If yes, we should note the expected perf impact in the PR.
pallets/permission0/api/src/lib.rs (1)
196-198: Nit: stray space before semicolon in runtime API signature.Keeps formatting clean for generated API surfaces.
- fn available_namespace_instances(permission_id: PermissionId, path: pallet_torus0_api::NamespacePathInner) -> Result<u32, DispatchError> ; + fn available_namespace_instances(permission_id: PermissionId, path: pallet_torus0_api::NamespacePathInner) -> Result<u32, DispatchError>;pallets/permission0/src/ext/namespace_impl.rs (1)
161-204: Depth traversal: consider early-exit once a matching lineage is found.The loop walks all (grandparent_id, namespace_set) entries; once a match recurses, continuing to scan siblings is unnecessary. A small micro-optimization: return after the recursive call. Behavior remains unchanged.
- for (grandparent_id, namespace_set) in parent_scope.paths.iter() { + for (grandparent_id, namespace_set) in parent_scope.paths.iter() { if namespace_set.contains(namespace_path) { - return check_namespace_delegation_depth::<T>(/* ... */); + return check_namespace_delegation_depth::<T>(/* ... */); } for parent_namespace in namespace_set.iter() { if parent_namespace.is_parent_of(namespace_path) { - return check_namespace_delegation_depth::<T>(/* ... */); + return check_namespace_delegation_depth::<T>(/* ... */); } } }pallets/permission0/src/permission/namespace.rs (1)
106-123: available_instances: O(children) scan is fine; consider documenting complexity.Small doc note could prevent accidental usage in tight loops.
pallets/permission0/src/benchmarking.rs (2)
422-431: Duplicate element in BTreeSet is a no-op; clarify intent or remove.Inserting "agent.alice.n" twice into a BTreeSet deduplicates silently. If the goal was to model within-call duplicates, a set won’t capture it. Consider removing the duplicate or switching to a Vec before bounding if you need duplicates.
- let alice_bob_set = BTreeSet::from([ + let alice_bob_set = BTreeSet::from([ b"agent.alice.compute".to_vec().try_into().unwrap(), b"agent.alice.network".to_vec().try_into().unwrap(), b"agent.alice.storage".to_vec().try_into().unwrap(), b"agent.alice.n".to_vec().try_into().unwrap(), - b"agent.alice.n".to_vec().try_into().unwrap(), ]);
472-496: Hot-path loop is fine; minor nit to pre-allocate names.Creating acc_name in the loop reallocates each time; negligible here, but a small pre-allocation or format!("acc{i}") would be marginally cleaner.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
.helix/languages.toml(1 hunks)justfile(1 hunks)pallets/emission0/src/benchmarking.rs(1 hunks)pallets/emission0/src/weights.rs(3 hunks)pallets/governance/src/weights.rs(31 hunks)pallets/permission0/api/src/lib.rs(1 hunks)pallets/permission0/src/benchmarking.rs(6 hunks)pallets/permission0/src/ext/namespace_impl.rs(3 hunks)pallets/permission0/src/permission/namespace.rs(2 hunks)pallets/permission0/src/weights.rs(13 hunks)pallets/permission0/tests/namespace.rs(2 hunks)pallets/torus0/src/weights.rs(11 hunks)runtime/src/apis.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-21T22:09:27.601Z
Learnt from: CR
PR: renlabs-dev/torus-substrate#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-21T22:09:27.601Z
Learning: Run cargo fmt, fix all clippy warnings, run checks/tests/coverage before committing
Applied to files:
.helix/languages.tomljustfile
📚 Learning: 2025-09-14T14:23:01.957Z
Learnt from: saiintbrisson
PR: renlabs-dev/torus-substrate#141
File: pallets/permission0/src/permission/namespace.rs:45-63
Timestamp: 2025-09-14T14:23:01.957Z
Learning: In the permission0 pallet's instance system, overlapping namespace paths (like `.compute` and `.compute.h100`) are treated as separate sources of instances that contribute additively. If a permission has both paths with 2 instances each, then `.compute.h100` can be delegated 4 times total (2 derived from `.compute` + 2 derived from `.compute.h100` directly). This additive behavior is intentional design, not overcounting.
Applied to files:
pallets/permission0/api/src/lib.rsruntime/src/apis.rspallets/permission0/src/permission/namespace.rspallets/permission0/tests/namespace.rspallets/permission0/src/ext/namespace_impl.rs
📚 Learning: 2025-08-21T22:09:27.601Z
Learnt from: CR
PR: renlabs-dev/torus-substrate#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-21T22:09:27.601Z
Learning: Applies to pallets/*/benchmarking/**/*.rs : Benchmark all extrinsics for weight calculation
Applied to files:
pallets/torus0/src/weights.rspallets/permission0/src/weights.rspallets/emission0/src/weights.rspallets/governance/src/weights.rs
📚 Learning: 2025-08-21T22:09:27.601Z
Learnt from: CR
PR: renlabs-dev/torus-substrate#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-21T22:09:27.601Z
Learning: Applies to {pallets,runtime}/**/*.rs : Specify weight info for all extrinsics
Applied to files:
pallets/torus0/src/weights.rspallets/permission0/src/weights.rspallets/emission0/src/weights.rspallets/governance/src/weights.rs
🔇 Additional comments (13)
pallets/governance/src/weights.rs (1)
5-6: Weights refresh looks coherent with metadata changes.Header date and proof-size adjustments align; numeric parts track min-times correctly across entries. No action needed.
If convenient, re-run your benchmark recipe (just run-benchmarks) to confirm no local env skew before merging.
pallets/torus0/src/weights.rs (1)
71-75: add_stake reweighting LGTM.Updated min-time and reads/writes look consistent.
runtime/src/apis.rs (2)
474-491: Runtime API: available_namespace_instances implementation looks correct.Proper path validation, error mapping, and Namespace-scope check; returns additive instances per design.
Please confirm
NamespacePath::new_agentaccepts exactly the same representation clients use over RPC to avoid UX mismatches.
502-505: Minor style: local binding is fine.The
ownertemp aids readability; no behavior change.pallets/permission0/src/ext/namespace_impl.rs (1)
246-251: Depth and instance checks wired in the right place.Calling check_namespace_delegation_depth before check_available_instances is sensible; it prevents unnecessary accounting work on paths that will be rejected by depth.
Please confirm MAX_DELEGATION_DEPTH = 5 matches product intent (i.e., 5 links allowed, 6th rejected).
pallets/permission0/tests/namespace.rs (4)
125-137: Solid negative test for empty state.Validates is_delegating_namespace returns false with no permissions.
139-169: Positive exact-match test looks good.Covers the intended happy path for the new API.
171-204: Parent/child relationship coverage is on point.Both directions exercised; this guards regressions in is_parent_of logic.
2028-2203: Comprehensive overlap/instance tests match intended additive semantics.The scenarios clearly encode the “overlapping sources additively contribute” design discussed previously. This is great coverage.
Please add a brief comment at the top referencing the additive-instance design decision (CHAIN-133) to help future readers understand why duplicates/overlaps are expected to succeed up to instance limits.
pallets/permission0/src/permission/namespace.rs (2)
32-53: redelegated_paths: implementation aligns with additive-source model.Collecting per-child paths keyed by this_id and pairing with child max_instances supports the additive accounting strategy.
63-104: check_available_instances: logic sound; in-call overlap handling is correct.Appending the current path into the working set to account for within-call overlaps matches the documented behavior.
pallets/permission0/src/weights.rs (1)
181-201: Rebenchmarked weights reflect heavier namespace delegation path.Reads/writes and proof sizes updated; looks consistent with the new validation and accounting.
If CI runners differ from the machine in the header, please re-run benchmarks on the canonical env to avoid drift in committed weights.
pallets/permission0/src/benchmarking.rs (1)
313-321: Bench setup: concise bounded-set construction looks good.The BTreeSet -> BoundedBTreeSet flow is clear and idiomatic for benchmarks.
|
are we gonna add the RPC of how many instances are available for path X in permission Y ? @saiintbrisson |
Yes, it's there already https://github.com/renlabs-dev/torus-substrate/pull/141/files#diff-ec502471379f95df97257dea99f93805c640509d276187e0b6f7fbbd98d8b75e |
|
2 participants
This patch expands on the instance system implementation. The old one was too limited, the new one allows for better flexibility when re-delegating subpaths.
Closes CHAIN-133.
Summary by CodeRabbit
New Features
Performance
Tests
Chores