Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion bin/evd/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -655,7 +655,11 @@ fn run_custom_genesis<S: ReadonlyKV + Storage>(
(addr.into_array(), acc.balance)
})
.collect();
let minter = AccountId::new(genesis_config.minter_id);
let minter = {
let mut b = [0u8; 32];
b[16..32].copy_from_slice(&genesis_config.minter_id.to_be_bytes());
AccountId::from_bytes(b)
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's find the genesis_config.rs file to see the type definition
fd genesis_config.rs

Repository: 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 -20

Repository: 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 || true

Repository: 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 -50

Repository: evstack/ev-rs

Length of output: 83


🏁 Script executed:

rg -B2 -A10 "struct AccountId|impl AccountId" --type-list | head -5

Repository: evstack/ev-rs

Length of output: 446


🏁 Script executed:

# Search for AccountId in all files
fd -e rs -x rg -l "struct AccountId" | head -5

Repository: evstack/ev-rs

Length of output: 247


🏁 Script executed:

# Try to find AccountId definition
rg -B2 -A10 "pub struct AccountId|impl AccountId" | head -80

Repository: 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 -50

Repository: evstack/ev-rs

Length of output: 2137


🏁 Script executed:

rg -A5 "fn from_u64" crates/app/sdk/core/src/lib.rs

Repository: 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 -50

Repository: 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)")
EOF

Repository: 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 -A2

Repository: 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 -A2

Repository: 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 -A5

Repository: 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)`.

let metadata = genesis_config.token.to_metadata();

let gas_config = default_gas_config();
Expand Down
4 changes: 2 additions & 2 deletions bin/testapp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use evolve_token::account::{Token, TokenRef};
use evolve_tx_eth::TxContext;
use evolve_tx_eth::{register_runtime_contract_account, resolve_or_create_eoa_account};

pub const MINTER: AccountId = AccountId::new(100_002);
pub const MINTER: AccountId = AccountId::from_u64(100002);

pub struct MempoolNoOpPostTx;

Expand All @@ -42,7 +42,7 @@ pub type MempoolStf = Stf<
MempoolNoOpPostTx,
>;

pub const PLACEHOLDER_ACCOUNT: AccountId = AccountId::new(u128::MAX);
pub const PLACEHOLDER_ACCOUNT: AccountId = AccountId::from_u64(u64::MAX);

/// Default gas configuration for the test application.
pub fn default_gas_config() -> StorageGasConfig {
Expand Down
6 changes: 5 additions & 1 deletion bin/testapp/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,11 @@ fn run_custom_genesis<S: ReadonlyKV + Storage>(
return Err("custom genesis requires at least one account".into());
}

let minter = AccountId::new(config.minter_id);
let minter = {
let mut b = [0u8; 32];
b[16..32].copy_from_slice(&config.minter_id.to_be_bytes());
AccountId::from_bytes(b)
};
let metadata = config.token.to_metadata();

let genesis_block = BlockContext::new(0, 0);
Expand Down
2 changes: 1 addition & 1 deletion bin/testapp/tests/mempool_e2e.rs
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ fn setup_genesis(

let init_storage = AsyncMockStorage::new();
let gas_config = default_gas_config();
let stf = build_mempool_stf(gas_config.clone(), AccountId::new(0));
let stf = build_mempool_stf(gas_config.clone(), AccountId::from_u64(0));

let (genesis_state, genesis_accounts) = do_eth_genesis(
&stf,
Expand Down
2 changes: 1 addition & 1 deletion bin/txload/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,7 @@ mod tests {

#[test]
fn build_transfer_calldata_has_selector_and_borsh_args() {
let recipient = AccountId::new(42);
let recipient = AccountId::from_u64(42);
let amount = 999_u128;

let calldata = build_transfer_calldata(recipient, amount).unwrap();
Expand Down
1 change: 1 addition & 0 deletions crates/app/genesis/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ evolve_stf = { workspace = true }
evolve_stf_traits = { workspace = true }

borsh = { workspace = true }
hex = "0.4"
serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0"
thiserror = "1.0"
Expand Down
149 changes: 94 additions & 55 deletions crates/app/genesis/src/file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ pub struct GenesisTxJson {
pub id: Option<String>,

/// Sender specification.
/// - "system": Use the system account (AccountId::new(0))
/// - "system": Use the system account (AccountId::from_u64(0))
/// - "$ref": Reference a previously created account
/// - number: Use a specific account ID
#[serde(default = "default_sender")]
Expand All @@ -64,15 +64,25 @@ fn default_sender() -> SenderSpec {
}

/// Specification for the sender of a genesis transaction.
#[derive(Debug, Clone, Default, Serialize)]
#[derive(Debug, Clone, Default, PartialEq, Eq)]
pub enum SenderSpec {
/// Use the system account
#[default]
System,
/// Reference a previously created account (starts with $)
Reference(String),
/// Use a specific account ID
AccountId(u128),
AccountId(AccountId),
}

impl Serialize for SenderSpec {
fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
match self {
SenderSpec::System => serializer.serialize_str("system"),
SenderSpec::Reference(r) => serializer.serialize_str(r),
SenderSpec::AccountId(id) => serializer.serialize_str(&hex::encode(id.to_bytes())),
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

impl<'de> serde::Deserialize<'de> for SenderSpec {
Expand All @@ -86,25 +96,36 @@ impl<'de> serde::Deserialize<'de> for SenderSpec {
match value {
serde_json::Value::String(s) if s == "system" => Ok(SenderSpec::System),
serde_json::Value::String(s) => Ok(SenderSpec::Reference(s)),
serde_json::Value::Number(n) => n
.as_u64()
.map(|v| SenderSpec::AccountId(v as u128))
.or_else(|| n.as_i64().map(|v| SenderSpec::AccountId(v as u128)))
.ok_or_else(|| D::Error::custom("invalid account ID number")),
serde_json::Value::Number(n) => {
let v = n
.as_u64()
.ok_or_else(|| D::Error::custom("invalid account ID number"))?;
Ok(SenderSpec::AccountId(AccountId::from_u64(v)))
}
_ => Err(D::Error::custom("expected string or number for sender")),
}
}
}

/// Specification for the recipient of a genesis transaction.
#[derive(Debug, Clone, Serialize)]
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum RecipientSpec {
/// Use the runtime account for account creation
Runtime,
/// Reference a previously created account (starts with $)
Reference(String),
/// Use a specific account ID
AccountId(u128),
AccountId(AccountId),
}

impl Serialize for RecipientSpec {
fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
match self {
RecipientSpec::Runtime => serializer.serialize_str("runtime"),
RecipientSpec::Reference(r) => serializer.serialize_str(r),
RecipientSpec::AccountId(id) => serializer.serialize_str(&hex::encode(id.to_bytes())),
}
}
}

impl<'de> serde::Deserialize<'de> for RecipientSpec {
Expand All @@ -118,11 +139,12 @@ impl<'de> serde::Deserialize<'de> for RecipientSpec {
match value {
serde_json::Value::String(s) if s == "runtime" => Ok(RecipientSpec::Runtime),
serde_json::Value::String(s) => Ok(RecipientSpec::Reference(s)),
serde_json::Value::Number(n) => n
.as_u64()
.map(|v| RecipientSpec::AccountId(v as u128))
.or_else(|| n.as_i64().map(|v| RecipientSpec::AccountId(v as u128)))
.ok_or_else(|| D::Error::custom("invalid account ID number")),
serde_json::Value::Number(n) => {
let v = n
.as_u64()
.ok_or_else(|| D::Error::custom("invalid account ID number"))?;
Ok(RecipientSpec::AccountId(AccountId::from_u64(v)))
}
_ => Err(D::Error::custom("expected string or number for recipient")),
}
}
Expand Down Expand Up @@ -189,27 +211,31 @@ impl GenesisFile {

let mut txs = Vec::with_capacity(self.transactions.len());
let mut id_to_account: HashMap<String, AccountId> = HashMap::new();
let mut next_account_id: u128 = 1; // Start from 1, 0 is system
// Start from 1, 0 is system
let mut next_account_id = AccountId::from_u64(1);

for (idx, tx_json) in self.transactions.iter().enumerate() {
// Resolve sender
let sender =
match &tx_json.sender {
SenderSpec::System => SYSTEM_ACCOUNT_ID,
SenderSpec::Reference(r) if r.starts_with('$') => {
let ref_id = &r[1..];
*id_to_account.get(ref_id).ok_or_else(|| {
GenesisError::InvalidReference(ref_id.to_string(), idx)
})?
}
SenderSpec::Reference(r) => {
// Treat as account ID string
AccountId::new(r.parse().map_err(|_| {
let sender = match &tx_json.sender {
SenderSpec::System => SYSTEM_ACCOUNT_ID,
SenderSpec::Reference(r) if r.starts_with('$') => {
let ref_id = &r[1..];
*id_to_account
.get(ref_id)
.ok_or_else(|| GenesisError::InvalidReference(ref_id.to_string(), idx))?
}
SenderSpec::Reference(r) => {
// Treat as hex-encoded 32-byte account ID
let bytes = hex::decode(r.trim_start_matches("0x"))
.ok()
.and_then(|b| <[u8; 32]>::try_from(b).ok())
.ok_or_else(|| {
GenesisError::ParseError(format!("invalid sender: {}", r))
})?)
}
SenderSpec::AccountId(id) => AccountId::new(*id),
};
})?;
AccountId::from_bytes(bytes)
}
SenderSpec::AccountId(id) => *id,
};

// Resolve recipient
let recipient = match &tx_json.recipient {
Expand All @@ -221,12 +247,16 @@ impl GenesisFile {
.ok_or_else(|| GenesisError::InvalidReference(ref_id.to_string(), idx))?
}
RecipientSpec::Reference(r) => {
// Treat as account ID string
AccountId::new(r.parse().map_err(|_| {
GenesisError::ParseError(format!("invalid recipient: {}", r))
})?)
// Treat as hex-encoded 32-byte account ID
let bytes = hex::decode(r.trim_start_matches("0x"))
.ok()
.and_then(|b| <[u8; 32]>::try_from(b).ok())
.ok_or_else(|| {
GenesisError::ParseError(format!("invalid recipient: {}", r))
})?;
AccountId::from_bytes(bytes)
}
RecipientSpec::AccountId(id) => AccountId::new(*id),
RecipientSpec::AccountId(id) => *id,
};

// Resolve $references in the message payload
Expand All @@ -244,9 +274,8 @@ impl GenesisFile {
// If this transaction has an ID, assign it the next account ID
// This assumes the transaction creates an account
if let Some(id) = &tx_json.id {
let account_id = AccountId::new(next_account_id);
id_to_account.insert(id.clone(), account_id);
next_account_id += 1;
id_to_account.insert(id.clone(), next_account_id);
next_account_id = next_account_id.increase();
}

txs.push(genesis_tx);
Expand All @@ -267,8 +296,9 @@ fn resolve_references_in_value(
let account_id = id_to_account
.get(ref_id)
.ok_or_else(|| GenesisError::InvalidReference(ref_id.to_string(), 0))?;
// Use string representation for u128 since JSON numbers are limited to i64/u64
Ok(serde_json::Value::String(account_id.inner().to_string()))
Ok(serde_json::Value::String(hex::encode(
account_id.as_bytes(),
)))
}
serde_json::Value::Array(arr) => {
let resolved: Result<Vec<_>, _> = arr
Expand Down Expand Up @@ -360,7 +390,7 @@ mod tests {
#[test]
fn test_resolve_references_in_value() {
let mut id_to_account = HashMap::new();
id_to_account.insert("alice".to_string(), AccountId::new(1));
id_to_account.insert("alice".to_string(), AccountId::from_u64(1));

let value = serde_json::json!({
"account": "$alice",
Expand All @@ -372,10 +402,19 @@ mod tests {

let resolved = resolve_references_in_value(&value, &id_to_account).unwrap();

// Account IDs are resolved as strings (for u128 compatibility)
assert_eq!(resolved["account"], "1");
assert_eq!(resolved["nested"]["ref"], "1");
assert_eq!(resolved["list"][0], "1");
// Account IDs are resolved as hex strings
assert_eq!(
resolved["account"],
"0000000000000000000000000000000000000000000000000000000000000001"
);
assert_eq!(
resolved["nested"]["ref"],
"0000000000000000000000000000000000000000000000000000000000000001"
);
assert_eq!(
resolved["list"][0],
"0000000000000000000000000000000000000000000000000000000000000001"
);
assert_eq!(resolved["list"][1], "literal");
}

Expand Down Expand Up @@ -459,10 +498,10 @@ mod tests {
]
}"#;
let genesis = GenesisFile::parse(json).unwrap();
assert!(matches!(
assert_eq!(
genesis.transactions[0].sender,
SenderSpec::AccountId(123)
));
SenderSpec::AccountId(AccountId::from_u64(123))
);
}

#[test]
Expand All @@ -488,10 +527,10 @@ mod tests {
]
}"#;
let genesis = GenesisFile::parse(json).unwrap();
assert!(matches!(
assert_eq!(
genesis.transactions[0].recipient,
RecipientSpec::AccountId(456)
));
RecipientSpec::AccountId(AccountId::from_u64(456))
);
}

#[test]
Expand Down Expand Up @@ -577,8 +616,8 @@ mod tests {
let txs = genesis.to_transactions(&registry).unwrap();

assert_eq!(txs.len(), 3);
assert_eq!(txs[2].sender, AccountId::new(1));
assert_eq!(txs[2].recipient, AccountId::new(1));
assert_eq!(txs[2].sender, AccountId::from_u64(1));
assert_eq!(txs[2].recipient, AccountId::from_u64(1));
}

#[test]
Expand Down
16 changes: 8 additions & 8 deletions crates/app/genesis/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ use std::cell::RefCell;
/// System account ID used as sender for genesis transactions.
/// This is a sentinel value that indicates the transaction originates
/// from the genesis process rather than a real account.
pub const SYSTEM_ACCOUNT_ID: AccountId = AccountId::new(0);
pub const SYSTEM_ACCOUNT_ID: AccountId = AccountId::from_u64(0);

/// Applies genesis transactions to initialize chain state.
///
Expand Down Expand Up @@ -441,7 +441,7 @@ mod tests {
let mut codes = CodeStore::new();
codes.add_code(TestAccount);

let test_account_id = AccountId::new(100);
let test_account_id = AccountId::from_u64(100);
let storage = setup_storage_with_account(test_account_id, "test_account");
let consensus_params = ConsensusParams::default();

Expand All @@ -467,8 +467,8 @@ mod tests {
let mut codes = CodeStore::new();
codes.add_code(TestAccount);

let account1 = AccountId::new(100);
let account2 = AccountId::new(101);
let account1 = AccountId::from_u64(100);
let account2 = AccountId::from_u64(101);
let mut storage = setup_storage_with_account(account1, "test_account");
let consensus_params = ConsensusParams::default();
// Add second account
Expand Down Expand Up @@ -517,9 +517,9 @@ mod tests {
codes.add_code(TestAccount);
codes.add_code(FailingAccount);

let good_account = AccountId::new(100);
let bad_account = AccountId::new(101);
let after_bad = AccountId::new(102);
let good_account = AccountId::from_u64(100);
let bad_account = AccountId::from_u64(101);
let after_bad = AccountId::from_u64(102);
let consensus_params = ConsensusParams::default();

let mut storage = setup_storage_with_account(good_account, "test_account");
Expand Down Expand Up @@ -585,7 +585,7 @@ mod tests {
let mut codes = CodeStore::new();
codes.add_code(FailingAccount);

let bad_account = AccountId::new(100);
let bad_account = AccountId::from_u64(100);
let storage = setup_storage_with_account(bad_account, "failing_account");
let consensus_params = ConsensusParams::default();

Expand Down
2 changes: 1 addition & 1 deletion crates/app/genesis/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ pub struct GenesisTx {
pub id: Option<String>,

/// The sender account ID.
/// For genesis, this is typically the system account (AccountId::new(0))
/// For genesis, this is typically the system account (AccountId::from_u64(0))
/// or a virtual sender specified in the genesis file.
pub sender: AccountId,

Expand Down
Loading
Loading