Skip to content
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.

Sign up for GitHub

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

fix(katana): include salt in genesis accounts #3031

Merged
merged 2 commits into from
Feb 14, 2025
Merged

fix(katana): include salt in genesis accounts #3031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
11419e0
include salt in genesis account struct
kariy Feb 14, 2025
d412c03
add more tests
kariy Feb 14, 2025
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
3 changes: 2 additions & 1 deletion bin/katana/src/cli/init/slot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,14 @@ pub fn add_paymasters_to_genesis(
let class_hash = DEFAULT_ACCOUNT_CLASS_HASH;
let balance = U256::from(DEFAULT_PREFUNDED_ACCOUNT_BALANCE);

let (addr, account) = GenesisAccount::new_with_salt_and_balance(
let account = GenesisAccount::new_with_salt_and_balance(
paymaster.public_key,
class_hash,
paymaster.salt,
balance,
);

let addr = account.address();
let account = GenesisAllocation::Account(GenesisAccountAlloc::Account(account));
accounts.push((addr, account));
}
Expand Down
2 changes: 2 additions & 0 deletions crates/katana/chain-spec/src/dev.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,7 @@ mod tests {
(felt!("0x1"), felt!("0x1")),
(felt!("0x2"), felt!("0x2")),
])),
salt: GenesisAccount::DEFAULT_SALT,
})),
),
(
Expand All @@ -329,6 +330,7 @@ mod tests {
class_hash: DEFAULT_ACCOUNT_CLASS_HASH,
nonce: None,
storage: None,
salt: GenesisAccount::DEFAULT_SALT,
})),
),
];
Expand Down
155 changes: 113 additions & 42 deletions crates/katana/chain-spec/src/rollup/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ use katana_primitives::class::{ClassHash, ContractClass};
use katana_primitives::contract::{ContractAddress, Nonce};
use katana_primitives::genesis::allocation::{DevGenesisAccount, GenesisAccountAlloc};
use katana_primitives::genesis::constant::{
DEFAULT_ACCOUNT_CLASS, DEFAULT_LEGACY_ERC20_CLASS, DEFAULT_LEGACY_UDC_CLASS,
GENESIS_ACCOUNT_CLASS,
DEFAULT_ACCOUNT_CLASS, DEFAULT_ACCOUNT_CLASS_HASH, DEFAULT_LEGACY_ERC20_CLASS,
DEFAULT_LEGACY_UDC_CLASS, GENESIS_ACCOUNT_CLASS,
};
use katana_primitives::transaction::{
DeclareTx, DeclareTxV0, DeclareTxV2, DeclareTxWithClass, DeployAccountTx, DeployAccountTxV1,
Expand Down Expand Up @@ -129,13 +129,12 @@ impl<'c> GenesisTransactionsBuilder<'c> {
class_hash
}

fn deploy(&self, class: ClassHash, ctor_args: Vec<Felt>) -> ContractAddress {
fn deploy(&self, class: ClassHash, ctor_args: Vec<Felt>, salt: Felt) -> ContractAddress {
use std::iter;

const DEPLOY_CONTRACT_SELECTOR: &str = "deploy_contract";
let master_address = *self.master_address.get().expect("must be initialized first");

let salt = Felt::ZERO;
let contract_address = get_contract_address(salt, class, &ctor_args, Felt::ZERO);

let ctor_args_len = Felt::from_usize(ctor_args.len()).unwrap();
Expand Down Expand Up @@ -187,25 +186,19 @@ impl<'c> GenesisTransactionsBuilder<'c> {
});
}

fn deploy_predeployed_account(&self, account: &DevGenesisAccount) -> ContractAddress {
// The salt used in `GenesisAccount::new()` to compute the contract address
//
// The only reason we use this value is to make sure the generated account addresses are the
// same with the previous implementation.
const SALT: Felt = felt!("666");

fn deploy_predeployed_dev_account(&self, account: &DevGenesisAccount) -> ContractAddress {
let signer = SigningKey::from_secret_scalar(account.private_key);
let pubkey = signer.verifying_key().scalar();

let class_hash = account.class_hash;
let calldata = vec![pubkey];
let account_address = get_contract_address(SALT, class_hash, &calldata, Felt::ZERO);
let account_address = get_contract_address(account.salt, class_hash, &calldata, Felt::ZERO);

let tx_hash = compute_deploy_account_v1_tx_hash(
account_address,
&calldata,
class_hash,
SALT,
account.salt,
0,
self.chain_spec.id.into(),
Felt::ZERO,
Expand All @@ -219,7 +212,7 @@ impl<'c> GenesisTransactionsBuilder<'c> {
contract_address: account_address.into(),
constructor_calldata: calldata,
chain_id: self.chain_spec.id,
contract_address_salt: SALT,
contract_address_salt: account.salt,
nonce: Felt::ZERO,
max_fee: 0,
class_hash,
Expand All @@ -231,6 +224,10 @@ impl<'c> GenesisTransactionsBuilder<'c> {
account_address.into()
}

fn deploy_predeployed_account(&self, salt: Felt, public_key: Felt) -> ContractAddress {
self.deploy(DEFAULT_ACCOUNT_CLASS_HASH, vec![public_key], salt)
}
Comment on lines +227 to +229
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

❓ Verification inconclusive

Please add test coverage for deploy_predeployed_account sensei.

The static analysis indicates that this method is not covered by tests.

🏁 Script executed:

#!/bin/bash
# Description: Check test coverage for deploy_predeployed_account method
rg -A 5 'deploy_predeployed_account.*public_key: Felt' -g '*test*.rs'

Length of output: 71

Ohayo sensei! It appears that the deploy_predeployed_account method isn’t currently covered by any tests. Since our verification script didn’t turn up any references in the test files, please add the necessary test coverage to ensure this functionality behaves as expected.

🏁 Script executed:

#!/bin/bash
# Searching for any references to deploy_predeployed_account in test files
rg -n 'deploy_predeployed_account' -g '*test*.rs'

Length of output: 49

Ohayo sensei! It looks like the deploy_predeployed_account method isn’t covered by any tests yet. Could you add some test cases for this function? Ensuring proper test coverage here will help maintain robust functionality for our rollup deployment logic.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 227-229: crates/katana/chain-spec/src/rollup/utils.rs#L227-L229
Added lines #L227 - L229 were not covered by tests


fn build_master_account(&self) {
let account_class_hash = self.legacy_declare(GENESIS_ACCOUNT_CLASS.clone());

Expand Down Expand Up @@ -272,7 +269,7 @@ impl<'c> GenesisTransactionsBuilder<'c> {

fn build_core_contracts(&mut self) {
let udc_class_hash = self.legacy_declare(DEFAULT_LEGACY_UDC_CLASS.clone());
self.deploy(udc_class_hash, Vec::new());
self.deploy(udc_class_hash, Vec::new(), Felt::ZERO);

let master_address = *self.master_address.get().expect("must be initialized first");

Expand All @@ -286,12 +283,12 @@ impl<'c> GenesisTransactionsBuilder<'c> {
];

let erc20_class_hash = self.legacy_declare(DEFAULT_LEGACY_ERC20_CLASS.clone());
let fee_token_address = self.deploy(erc20_class_hash, ctor_args);
let fee_token_address = self.deploy(erc20_class_hash, ctor_args, Felt::ZERO);

self.fee_token.set(fee_token_address).expect("must be uninitialized");
}

fn build_allocated_dev_accounts(&mut self) {
fn build_allocated_accounts(&mut self) {
let default_account_class_hash = self.declare(DEFAULT_ACCOUNT_CLASS.clone());

for (expected_addr, account) in self.chain_spec.genesis.accounts() {
Expand All @@ -303,12 +300,19 @@ impl<'c> GenesisTransactionsBuilder<'c> {
)
}

if let GenesisAccountAlloc::DevAccount(account) = account {
let account_address = self.deploy_predeployed_account(account);
debug_assert_eq!(&account_address, expected_addr);
if let Some(amount) = account.balance {
self.transfer_balance(account_address, amount);
let address = match account {
GenesisAccountAlloc::DevAccount(account) => {
self.deploy_predeployed_dev_account(account)
}
GenesisAccountAlloc::Account(account) => {
self.deploy_predeployed_account(account.salt, account.public_key)
}
};

debug_assert_eq!(&address, expected_addr);

if let Some(amount) = account.balance() {
self.transfer_balance(address, amount)
}
}
}
Expand All @@ -326,7 +330,7 @@ impl<'c> GenesisTransactionsBuilder<'c> {
pub fn build(mut self) -> Vec<ExecutableTxWithHash> {
self.build_master_account();
self.build_core_contracts();
self.build_allocated_dev_accounts();
self.build_allocated_accounts();
self.transactions.into_inner()
}
}
Expand All @@ -338,15 +342,19 @@ mod tests {
use katana_executor::implementation::blockifier::BlockifierFactory;
use katana_executor::{BlockLimits, ExecutorFactory};
use katana_primitives::chain::ChainId;
use katana_primitives::class::ClassHash;
use katana_primitives::contract::Nonce;
use katana_primitives::env::CfgEnv;
use katana_primitives::genesis::allocation::DevAllocationsGenerator;
use katana_primitives::genesis::allocation::{
DevAllocationsGenerator, GenesisAccount, GenesisAccountAlloc, GenesisAllocation,
};
use katana_primitives::genesis::constant::{
DEFAULT_LEGACY_ERC20_CLASS, DEFAULT_LEGACY_UDC_CLASS, DEFAULT_PREFUNDED_ACCOUNT_BALANCE,
DEFAULT_UDC_ADDRESS,
DEFAULT_ACCOUNT_CLASS_HASH, DEFAULT_LEGACY_ERC20_CLASS, DEFAULT_LEGACY_UDC_CLASS,
DEFAULT_PREFUNDED_ACCOUNT_BALANCE, DEFAULT_UDC_ADDRESS,
};
use katana_primitives::genesis::Genesis;
use katana_primitives::transaction::TxType;
use katana_primitives::Felt;
use katana_provider::providers::db::DbProvider;
use katana_provider::traits::state::StateFactoryProvider;
use url::Url;
Expand All @@ -355,10 +363,14 @@ mod tests {
use crate::rollup::{ChainSpec, FeeContract, DEFAULT_APPCHAIN_FEE_TOKEN_ADDRESS};
use crate::SettlementLayer;

fn chain_spec(n_accounts: u16) -> ChainSpec {
let accounts = DevAllocationsGenerator::new(n_accounts)
.with_balance(U256::from(DEFAULT_PREFUNDED_ACCOUNT_BALANCE))
.generate();
fn chain_spec(n_dev_accounts: u16, with_balance: bool) -> ChainSpec {
let accounts = if with_balance {
DevAllocationsGenerator::new(n_dev_accounts)
.with_balance(U256::from(DEFAULT_PREFUNDED_ACCOUNT_BALANCE))
.generate()
} else {
DevAllocationsGenerator::new(n_dev_accounts).generate()
};

let mut genesis = Genesis::default();
genesis.extend_allocations(accounts.into_iter().map(|(k, v)| (k, v.into())));
Expand Down Expand Up @@ -393,7 +405,7 @@ mod tests {

#[test]
fn valid_transactions() {
let chain_spec = chain_spec(1);
let chain_spec = chain_spec(1, true);

let provider = DbProvider::new_ephemeral();
let ef = executor(&chain_spec);
Expand All @@ -410,7 +422,7 @@ mod tests {

#[test]
fn genesis_states() {
let chain_spec = chain_spec(1);
let chain_spec = chain_spec(1, true);

let provider = DbProvider::new_ephemeral();
let ef = executor(&chain_spec);
Expand Down Expand Up @@ -453,7 +465,7 @@ mod tests {

#[test]
fn transaction_order() {
let chain_spec = chain_spec(1);
let chain_spec = chain_spec(1, true);
let transactions = GenesisTransactionsBuilder::new(&chain_spec).build();

let expected_order = vec![
Expand All @@ -474,27 +486,86 @@ mod tests {
}
}

#[test]
fn dev_predeployed_acccounts() {
fn inner(n_accounts: u16) {
let chain_spec = chain_spec(n_accounts);
#[rstest::rstest]
#[case::with_balance(true)]
#[case::no_balance(false)]
fn predeployed_acccounts(#[case] with_balance: bool) {
fn inner(n_accounts: usize, with_balance: bool) {
let mut chain_spec = chain_spec(0, with_balance);

// add non-dev allocations
for i in 0..n_accounts {
const CLASS_HASH: ClassHash = DEFAULT_ACCOUNT_CLASS_HASH;
let salt = Felt::from(i);
let pk = Felt::from(1337);

let mut account = GenesisAccount::new_with_salt(pk, CLASS_HASH, salt);

if with_balance {
account.balance = Some(U256::from(DEFAULT_PREFUNDED_ACCOUNT_BALANCE));
}

chain_spec.genesis.extend_allocations([(
account.address(),
GenesisAllocation::Account(GenesisAccountAlloc::Account(account)),
)]);
}

let mut transactions = GenesisTransactionsBuilder::new(&chain_spec).build();

// We only want to check that for each predeployed accounts, there should be a deploy
// account and transfer balance (invoke) transactions. So we skip the first 7
// transactions (master account, UDC, ERC20, etc).
let account_transactions = &transactions.split_off(7);

assert_eq!(account_transactions.len(), n_accounts as usize * 2);
if with_balance {
assert_eq!(account_transactions.len(), n_accounts * 2);
for txs in account_transactions.chunks(2) {
assert_eq!(txs[0].transaction.r#type(), TxType::Invoke); // deploy
assert_eq!(txs[1].transaction.r#type(), TxType::Invoke); // transfer
}
} else {
assert_eq!(account_transactions.len(), n_accounts);
for txs in account_transactions.chunks(2) {
assert_eq!(txs[0].transaction.r#type(), TxType::Invoke); // deploy
}
}
}

for i in 0..10 {
inner(i, with_balance);
}
}

#[rstest::rstest]
#[case::with_balance(true)]
#[case::no_balance(false)]
fn dev_predeployed_acccounts(#[case] with_balance: bool) {
fn inner(n_accounts: u16, with_balance: bool) {
let chain_spec = chain_spec(n_accounts, with_balance);
let mut transactions = GenesisTransactionsBuilder::new(&chain_spec).build();

// We only want to check that for each predeployed accounts, there should be a deploy
// account and transfer balance (invoke) transactions. So we skip the first 7
// transactions (master account, UDC, ERC20, etc).
let account_transactions = &transactions.split_off(7);

for txs in account_transactions.chunks(2) {
assert_eq!(txs[0].transaction.r#type(), TxType::DeployAccount);
assert_eq!(txs[1].transaction.r#type(), TxType::Invoke);
if with_balance {
assert_eq!(account_transactions.len(), n_accounts as usize * 2);
for txs in account_transactions.chunks(2) {
assert_eq!(txs[0].transaction.r#type(), TxType::DeployAccount);
assert_eq!(txs[1].transaction.r#type(), TxType::Invoke); // transfer
}
} else {
assert_eq!(account_transactions.len(), n_accounts as usize);
for txs in account_transactions.chunks(2) {
assert_eq!(txs[0].transaction.r#type(), TxType::DeployAccount);
}
}
}

for i in 0..10 {
inner(i);
inner(i, with_balance);
}
}
}
Loading
Loading