Merged
Conversation
Replace all AccountId::new(N) usages in the 5 assigned core SDK files with the equivalent AccountId::from_bytes([...]) byte array literals: - storage_api.rs: STORAGE_ACCOUNT_ID constant (N=1) - unique_api.rs: UNIQUE_HANDLER_ACCOUNT_ID constant (N=5) - core/lib.rs: inline test AccountIds for 1, 2, 10, 42; updated test_account_id_u128_compat to assert on bytes instead of inner() - fungible_asset.rs: test AccountIds 1, 1111, 1234, 2222, 9999 converted to named const byte arrays - collections/mocks.rs: MockEnvironment::new keeps u128 params but constructs AccountId via from_bytes instead of AccountId::new Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…, and binaries Replaces all AccountId::new(N: u128) calls with AccountId::from_bytes([u8;32]) across: chain-index integration tests, rpc/types log tests, evnode run_server example, debugger (lib, replay, trace, breakpoints, inspector), storage warming tests, testapp lib constants (MINTER, PLACEHOLDER_ACCOUNT), testapp/evd run_custom_genesis minter_id runtime conversion, mempool_e2e tests, and txload test. The minter_id config field remains u128 (JSON-deserialized) with an inline conversion at the call site. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… standards Replace all AccountId::new(N: u128) calls with AccountId::from_bytes([u8;32]) across testing, authentication, fungible_asset standards, and token, scheduler, nonceless_account extensions. Also change setup_token() and setup_account() helper signatures from u128 params to AccountId to avoid internal new() usage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Completes TODO(account-id-cleanup). All callers have been migrated to AccountId::from_bytes([u8;32]) in the preceding commits. The two remaining string-fallback paths in genesis/file.rs now parse hex-encoded 32-byte account IDs instead of decimal u128 strings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors AccountId usage across the codebase: adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Add AccountId::from_u64(n: u64) const fn that encodes n in the last 8 bytes big-endian. Replace all byte literal patterns across the workspace with the cleaner from_u64(N) form. PLACEHOLDER_ACCOUNT migrated from u128::MAX to u64::MAX (still a never-real-account sentinel). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Eliminates raw 32-byte array literals from genesis spec types by storing AccountId directly. Simplifies deserializers to use from_u64 and adds manual Serialize impls to hex-encode the account ID. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/app/stf/src/runtime_api_impl.rs (1)
41-41: Prefer a named constant instead of hard-coding65535.Line 41 introduces a magic seed value. A constant makes the runtime account-number baseline explicit and safer to maintain.
♻️ Suggested cleanup
+const INITIAL_ACCOUNT_NUMBER_SEED: u64 = 65_535; + pub(crate) fn next_account_number<S: ReadonlyKV>( storage: &mut ExecutionState<S>, ) -> SdkResult<AccountId> { @@ let last = storage .get(&key)? .map(|msg| msg.get()) - .unwrap_or(Ok(AccountId::from_u64(65535)))?; + .unwrap_or(Ok(AccountId::from_u64(INITIAL_ACCOUNT_NUMBER_SEED)))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/stf/src/runtime_api_impl.rs` at line 41, The hard-coded seed 65535 should be replaced with a named constant to make the runtime account baseline explicit: define a constant (e.g., RUNTIME_ACCOUNT_BASE: u64 = 65535) near the top of the module (or as pub(crate) if needed) and use it in the expression currently calling AccountId::from_u64(65535) so the line becomes unwrap_or(Ok(AccountId::from_u64(RUNTIME_ACCOUNT_BASE)))?; keep the constant name descriptive and update any related comments to reflect the new constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/evd/src/main.rs`:
- Around line 658-662: The code builds `minter` by manual byte-copy from
`genesis_config.minter_id` which is u128 and can silently overflow the intended
u64 account space; replace that logic by validating `genesis_config.minter_id`
fits in u64 and then construct the AccountId via `AccountId::from_u64(...)`
(e.g., check `minter_id <= u64::MAX` and cast to u64 or return/panic on
out-of-range), removing the manual
`b[16..32].copy_from_slice(...)`/`AccountId::from_bytes` approach so the value
is normalized and bounded consistently with other usages like `const MINTER:
AccountId = AccountId::from_u64(100002)`.
In `@crates/app/sdk/collections/src/mocks.rs`:
- Around line 31-42: The mock constructor new currently accepts u128 and
manually packs bytes into AccountId; change its signature to accept u64 for both
account_id and sender_id, and replace the manual byte construction with
AccountId::from_u64(account_id) and AccountId::from_u64(sender_id) respectively
(update any uses inside the new function where AccountId::from_bytes is used).
Ensure any callers are updated to pass u64 if needed and keep the rest of the
struct initialization unchanged.
---
Nitpick comments:
In `@crates/app/stf/src/runtime_api_impl.rs`:
- Line 41: The hard-coded seed 65535 should be replaced with a named constant to
make the runtime account baseline explicit: define a constant (e.g.,
RUNTIME_ACCOUNT_BASE: u64 = 65535) near the top of the module (or as pub(crate)
if needed) and use it in the expression currently calling
AccountId::from_u64(65535) so the line becomes
unwrap_or(Ok(AccountId::from_u64(RUNTIME_ACCOUNT_BASE)))?; keep the constant
name descriptive and update any related comments to reflect the new constant.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
bin/evd/src/main.rsbin/testapp/src/lib.rsbin/testapp/src/main.rsbin/testapp/tests/mempool_e2e.rsbin/txload/src/main.rscrates/app/genesis/Cargo.tomlcrates/app/genesis/src/file.rscrates/app/genesis/src/lib.rscrates/app/genesis/src/types.rscrates/app/sdk/collections/src/mocks.rscrates/app/sdk/core/src/fungible_asset.rscrates/app/sdk/core/src/lib.rscrates/app/sdk/core/src/runtime_api.rscrates/app/sdk/core/src/storage_api.rscrates/app/sdk/core/src/unique_api.rscrates/app/sdk/extensions/nonceless_account/src/lib.rscrates/app/sdk/extensions/scheduler/src/lib.rscrates/app/sdk/extensions/token/src/lib.rscrates/app/sdk/standards/evolve_authentication/src/lib.rscrates/app/sdk/standards/evolve_fungible_asset/src/lib.rscrates/app/sdk/testing/src/lib.rscrates/app/stf/src/lib.rscrates/app/stf/src/runtime_api_impl.rscrates/app/tx/eth/src/traits.rscrates/rpc/chain-index/src/integration.rscrates/rpc/evnode/examples/run_server.rscrates/rpc/types/src/log.rscrates/storage/src/warming.rscrates/testing/debugger/src/breakpoints.rscrates/testing/debugger/src/inspector.rscrates/testing/debugger/src/lib.rscrates/testing/debugger/src/replay.rscrates/testing/debugger/src/trace.rs
bin/evd/src/main.rs
Outdated
Comment on lines
658
to
662
| let minter = { | ||
| let mut b = [0u8; 32]; | ||
| b[16..32].copy_from_slice(&genesis_config.minter_id.to_be_bytes()); | ||
| AccountId::from_bytes(b) | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the genesis_config.rs file to see the type definition
fd genesis_config.rsRepository: evstack/ev-rs
Length of output: 90
🏁 Script executed:
# Let's also check the structure of the codebase
git ls-files | grep -E "(genesis|account)" | head -20Repository: evstack/ev-rs
Length of output: 493
🏁 Script executed:
# Run the verification script from the review comment
fd genesis_config.rs -x rg -n "struct EvdGenesisConfig|minter_id"
rg -n "minter_id\.to_be_bytes|AccountId::from_bytes\(|AccountId::from_u64\(" \
bin/evd/src/main.rs \
bin/testapp/src/main.rs \
bin/testapp/src/lib.rs 2>/dev/null || trueRepository: evstack/ev-rs
Length of output: 977
🏁 Script executed:
# Let's also check the AccountId definition and available constructors
rg -A 5 "impl AccountId|fn from_u64|fn from_bytes" --type rs | head -50Repository: evstack/ev-rs
Length of output: 83
🏁 Script executed:
rg -B2 -A10 "struct AccountId|impl AccountId" --type-list | head -5Repository: evstack/ev-rs
Length of output: 446
🏁 Script executed:
# Search for AccountId in all files
fd -e rs -x rg -l "struct AccountId" | head -5Repository: evstack/ev-rs
Length of output: 247
🏁 Script executed:
# Try to find AccountId definition
rg -B2 -A10 "pub struct AccountId|impl AccountId" | head -80Repository: evstack/ev-rs
Length of output: 4312
🏁 Script executed:
# Look at the actual file structure for AccountId
cat -n bin/testapp/src/lib.rs | head -50Repository: evstack/ev-rs
Length of output: 2137
🏁 Script executed:
rg -A5 "fn from_u64" crates/app/sdk/core/src/lib.rsRepository: evstack/ev-rs
Length of output: 303
🏁 Script executed:
# Let's see more of the AccountId implementation
cat -n crates/app/sdk/core/src/lib.rs | sed -n '1,100p'Repository: evstack/ev-rs
Length of output: 3966
🏁 Script executed:
# Get more context around AccountId
cat -n crates/app/sdk/core/src/lib.rs | sed -n '1,80p'Repository: evstack/ev-rs
Length of output: 3280
🏁 Script executed:
# Check if there are other references to minter_id or from_u64 in the codebase
rg -B3 -A3 "AccountId::from_u64|from_u64" | head -50Repository: evstack/ev-rs
Length of output: 3159
🏁 Script executed:
# Look at the test data to understand what values are being used
cat -n bin/testapp/src/genesis_config.rs | sed -n '150,250p'Repository: evstack/ev-rs
Length of output: 4136
🏁 Script executed:
# Let's check the actual byte layout difference between from_u64 and the manual encoding
python3 << 'EOF'
# Show what from_u64 does vs what the current code does for a test value
test_value = 100
# from_u64 encoding (places u64 in last 8 bytes)
from_u64_bytes = [0] * 24 + list(test_value.to_bytes(8, 'big'))
print(f"from_u64({test_value}) bytes: {[f'{b:02x}' for b in from_u64_bytes]}")
# Current code with u128 (places u128 in last 16 bytes)
current_bytes = [0] * 16 + list(test_value.to_bytes(16, 'big'))
print(f"Current u128 code bytes: {[f'{b:02x}' for b in current_bytes]}")
print(f"\nAre they the same for value {test_value}? {from_u64_bytes == current_bytes}")
# Test with a large value that wouldn't fit in u64
large_value = 2**65 # Larger than u64::MAX
try:
large_u64_bytes = list(large_value.to_bytes(8, 'big'))
except:
print(f"\nValue {large_value} cannot fit in u64")
large_u128_bytes = [0] * 16 + list(large_value.to_bytes(16, 'big'))
print(f"But it fits in u128 (current code)")
EOFRepository: evstack/ev-rs
Length of output: 608
🏁 Script executed:
# Check if there are any usages that might pass large u128 values to minter_id
rg "minter_id\s*[:=]" -B2 -A2Repository: evstack/ev-rs
Length of output: 699
🏁 Script executed:
# Check if minter_id ever gets assigned values larger than u64::MAX in practice
rg "minter_id\s*[:=]\s*[0-9]" -B2 -A2Repository: evstack/ev-rs
Length of output: 403
🏁 Script executed:
# Look at what values are actually being used in tests and configuration
rg "minter_id.*100" bin/testapp/src/main.rs -B5 -A5Repository: evstack/ev-rs
Length of output: 481
Normalize minter_id via AccountId::from_u64 with explicit bounds checking.
The field minter_id is declared as u128, but the code should only accept u64 values to maintain consistency with the canonical AccountId constructor pattern used elsewhere (e.g., const MINTER: AccountId = AccountId::from_u64(100002) in bin/testapp/src/lib.rs). The current manual byte encoding at lines 660–661 silently accepts values exceeding u64::MAX without validation.
Suggested fix
- let minter = {
- let mut b = [0u8; 32];
- b[16..32].copy_from_slice(&genesis_config.minter_id.to_be_bytes());
- AccountId::from_bytes(b)
- };
+ let minter = AccountId::from_u64(
+ u64::try_from(genesis_config.minter_id)
+ .expect("genesis minter_id must fit in u64"),
+ );📝 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.
Suggested change
| let minter = { | |
| let mut b = [0u8; 32]; | |
| b[16..32].copy_from_slice(&genesis_config.minter_id.to_be_bytes()); | |
| AccountId::from_bytes(b) | |
| }; | |
| let minter = AccountId::from_u64( | |
| u64::try_from(genesis_config.minter_id) | |
| .expect("genesis minter_id must fit in u64"), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/evd/src/main.rs` around lines 658 - 662, The code builds `minter` by
manual byte-copy from `genesis_config.minter_id` which is u128 and can silently
overflow the intended u64 account space; replace that logic by validating
`genesis_config.minter_id` fits in u64 and then construct the AccountId via
`AccountId::from_u64(...)` (e.g., check `minter_id <= u64::MAX` and cast to u64
or return/panic on out-of-range), removing the manual
`b[16..32].copy_from_slice(...)`/`AccountId::from_bytes` approach so the value
is normalized and bounded consistently with other usages like `const MINTER:
AccountId = AccountId::from_u64(100002)`.
- Change minter_id from u128 to u64 in EvdGenesisConfig - Replace manual byte-packing with AccountId::from_u64 in evd and testapp - Fix serde round-trip for SenderSpec/RecipientSpec hex AccountId variants - Change MockEnvironment::new to accept u64 and use from_u64 - Extract INITIAL_ACCOUNT_NUMBER constant in runtime_api_impl Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Summary by CodeRabbit
Refactor
New Features