-
Notifications
You must be signed in to change notification settings - Fork 66
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.
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
Whitelist for Transaction Signers #826
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
use super::common::{ | ||
default_gc_slot_duration_ms, default_ingress_account_whitelist, default_sequence_number_ttl_ms, | ||
}; | ||
use aptos_types::account_address::AccountAddress; | ||
use serde::{Deserialize, Serialize}; | ||
use std::collections::HashSet; | ||
|
||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
pub struct Config { | ||
/// The number of milliseconds a sequence number is valid for. | ||
#[serde(default = "default_sequence_number_ttl_ms")] | ||
pub sequence_number_ttl_ms: u64, | ||
|
||
/// The duration of a garbage collection slot in milliseconds. | ||
#[serde(default = "default_gc_slot_duration_ms")] | ||
pub gc_slot_duration_ms: u64, | ||
|
||
/// The whitelist (path) for the mempool | ||
#[serde(default = "default_ingress_account_whitelist")] | ||
pub ingress_account_whitelist: Option<String>, | ||
} | ||
|
||
impl Default for Config { | ||
fn default() -> Self { | ||
Self { | ||
sequence_number_ttl_ms: default_sequence_number_ttl_ms(), | ||
gc_slot_duration_ms: default_gc_slot_duration_ms(), | ||
ingress_account_whitelist: default_ingress_account_whitelist(), | ||
} | ||
} | ||
} | ||
|
||
impl Config { | ||
pub fn whitelisted_accounts(&self) -> Result<Option<HashSet<AccountAddress>>, anyhow::Error> { | ||
match &self.ingress_account_whitelist { | ||
Some(whitelist_path) => { | ||
let mut whitelisted = HashSet::new(); | ||
|
||
// read the file from memory | ||
let file_string = String::from_utf8(std::fs::read(whitelist_path)?)?; | ||
Comment on lines
+39
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be done eagerly when config is initialized, not lazily in the getter which may be called multiple times. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally, yes, but that requires modifying much more throughout the codebase. The approach here is to assume that the config is only responsible for providing the whitelist file path. |
||
|
||
// for each line | ||
for line in file_string.lines() { | ||
let account = AccountAddress::from_hex(line.trim())?; | ||
whitelisted.insert(account); | ||
} | ||
|
||
Ok(Some(whitelisted)) | ||
} | ||
None => Ok(None), | ||
} | ||
} | ||
} |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to be fallible? Wouldn't it be better to have Config correct on construction and
whitelisted_accounts
a simple getter?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your second line of review answers this.