-
Notifications
You must be signed in to change notification settings - Fork 65
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?
Conversation
@@ -71,6 +75,14 @@ impl TransactionPipe { | |||
mempool_config.sequence_number_ttl_ms, | |||
mempool_config.gc_slot_duration_ms, | |||
), | |||
whitelisted_accounts: mempool_config.whitelisted_accounts()?, |
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.
// read the file from memory | ||
let file_string = String::from_utf8(std::fs::read(whitelist_path)?)?; |
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.
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 comment
The 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.
Summary
protocol-units
Enables a transaction signers whitelist.
TooManyTransactions
if the signer is not whitelisted.MAPTOS_INGRESS_ACCOUNT_WHITELIST
environment variable or directly in the Maptos Config.Testing
Outstanding issues
RocksDB
set.