Read general account files at genesis #1624
Conversation
This reverts commit a89b4c9.
| .map(|file_path| { | ||
| let toml_str = fs_err::read_to_string(file_path)?; | ||
| GenesisConfig::read_toml(toml_str.as_str()).with_context(|| { | ||
| GenesisConfig::read_toml_file(file_path).with_context(|| { |
There was a problem hiding this comment.
This swallows some errors, i.e. missing, access rights
| /// Generates sample agglayer account files for the `02-with-account-files` genesis config sample. | ||
| /// | ||
| /// Creates: | ||
| /// - `bridge.mac` - agglayer bridge account |
There was a problem hiding this comment.
Did we document the .mac extension somewhere? miden-account-code?
| # using deterministic seeds for reproducibility. | ||
| # They demonstrate interdependencies between accounts: |
There was a problem hiding this comment.
Nit
| # using deterministic seeds for reproducibility. | |
| # They demonstrate interdependencies between accounts: | |
| # using deterministic seeds for reproducibility. | |
| # | |
| # They demonstrate interdependencies between accounts: |
| AccountDelta(#[from] AccountDeltaError), | ||
| #[error("the defined asset {symbol:?} has no corresponding faucet")] | ||
| #[error( | ||
| "the defined asset {symbol:?} has no corresponding faucet, or the faucet was provided as an account file" |
There was a problem hiding this comment.
We should impl Fmt for TokenSymbolStr so we can do
| "the defined asset {symbol:?} has no corresponding faucet, or the faucet was provided as an account file" | |
| "the defined asset '{symbol}' has no corresponding faucet, or the faucet was provided as an account file" |
| #[error( | ||
| "the defined asset {symbol:?} has no corresponding faucet, or the faucet was provided as an account file" | ||
| )] |
There was a problem hiding this comment.
I don't understand how the faucet being specified as an account file is an error, but I'll return to this once I've read the rest.
There was a problem hiding this comment.
Why are generating them if the accounts are checked-in? As a general rule build.rs should be used for out of tree code generation, not to generate code that gets checked in.
There was a problem hiding this comment.
Out of scope: We should revisit how we deal protobuf generated code too.
| let toml_str = | ||
| fs_err::read_to_string(path).map_err(|e| GenesisConfigError::ConfigFileRead { | ||
| path: path.to_path_buf(), | ||
| reason: e.to_string(), |
There was a problem hiding this comment.
This will lose context because an errors ToString implementation generally only logs the last error and not the full back trace. This is why we generally avoid "stringly" errors.
| let account_file = AccountFile::read(&full_path).map_err(|e| { | ||
| GenesisConfigError::AccountFileRead { | ||
| path: full_path.clone(), | ||
| reason: e.to_string(), |
There was a problem hiding this comment.
This drops error context
| #[derive(Debug, Clone, serde::Deserialize)] | ||
| struct GenesisConfigToml { |
There was a problem hiding this comment.
Probably?
| #[derive(Debug, Clone, serde::Deserialize)] | |
| struct GenesisConfigToml { | |
| #[derive(Debug, Clone, serde::Deserialize)] | |
| #[serde(deny_unknown_fields)] | |
| struct GenesisConfigToml { |
|
|
||
| impl From<TokenSymbol> for TokenSymbolStr { | ||
| fn from(symbol: TokenSymbol) -> Self { | ||
| // TokenSymbol guarantees valid format, so to_string should not fail |
There was a problem hiding this comment.
| // TokenSymbol guarantees valid format, so to_string should not fail | |
| // SAFETY: TokenSymbol guarantees valid format, so to_string should not fail |
| // Append file-loaded accounts as-is | ||
| all_accounts.extend(file_loaded_accounts); |
There was a problem hiding this comment.
I don't think this is safe - do the accounts not need to be verified? e.g. they can contain assets which have no faucets.
| }, | ||
| Account { | ||
| account: Box<Account>, | ||
| }, |
There was a problem hiding this comment.
I think this is somewhat confusing because its not adding much over NativeFaucetToml apart from having the account parsed (sometimes).
I would remove this; or at least streamline/refactor things so they make more sense.
3 participants
Changes:
NativeFaucetan enum withParametersandAccountvariantsGenesisConfig::read_toml(toml_content)to `read_toml_path(toml_path),Pathinstead, so we can lookup the directory and read in account files accordingly, which are referenced relative to the config file02-with-account-files.toml.macfiles that correspond to AggLayer accounts. The.macfiles are also committedbuild.rsfile inconfig::storemodule that auto-generates theseAccountFilescloses #1429