-
Notifications
You must be signed in to change notification settings - Fork 42
Adding possibility to add more prefunded accounts #221
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
base: main
Are you sure you want to change the base?
Adding possibility to add more prefunded accounts #221
Conversation
Add possibility to add more prefunded accounts to the L1 generated genesis by accepting a repeated `[]string` flag `--prefunded-account` which accepts the same format (private key in hex) as the static prefunded accounts. I debated about putting the `prefunded-account` flag on the L1 recipe directly, to scope it just to this. Maybe it would be preferable, let me know what do you think.
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.
Pull request overview
This pull request adds the ability to specify additional prefunded accounts for L1 and L2 genesis configurations through a new --prefunded-account CLI flag. The flag accepts private keys in hexadecimal format and can be specified multiple times to add multiple accounts.
Key changes:
- New
--prefunded-accountflag that accepts private keys and can be repeated - Refactored static prefunded accounts into a separate variable positioned closer to usage
- Additional prefunded accounts are combined with the existing 10 static accounts in the genesis
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| playground/artifacts.go | Added prefundedAccounts field to ArtifactsBuilder, new setter method PrefundedAccounts(), helper method getPrefundedAccounts() to combine static and user-provided accounts, and moved static accounts definition for better code organization |
| main.go | Added prefundedAccounts global variable and --prefunded-account CLI flag, integrated the flag value with the artifacts builder |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Took the opportunity to document the prefunded
|
LGTM! |
|
@ferranbt Let me know how you want to proceed, will be happy to work any you like for this. |
|
LGTM. Can you please resolve the conflicts? |
|
@ferranbt Done |
canercidam
left a comment
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.
Thanks! I have a few very small requests.
| gen.Config.DepositContractAddress = gethcommon.HexToAddress(config.DepositContractAddress) | ||
|
|
||
| // add pre-funded accounts | ||
| prefundedBalance, _ := new(big.Int).SetString("10000000000000000000000", 16) |
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 number is making it hard to track balance movements. Usually if something is 1000 by default, 999 is an easy visual signal about a balance movement. Let's change this to base 10 and adapt the documentation.
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 is done like this to align with the other prefunded balances:
- https://github.com/flashbots/builder-playground/blob/main/playground/artifacts.go#L169
- https://github.com/flashbots/builder-playground/blob/main/playground/artifacts.go#L301
Do you want me to fully change all of them?
Usually if something is 1000 by default
I saw all numbers over the years, overly big doesn't change that much I would personally go with something bigger than smaller, just for those that don't we tiny amount of wei to perform their testing and input really big eth value.
Something like 99999999999999999 ETH.
All in all, let me know and I'll make the necessary work.
main.go
Outdated
| recipeCmd.Flags().BoolVar(&contenderEnabled, "contender", false, "spam nodes with contender") | ||
| recipeCmd.Flags().StringArrayVar(&contenderArgs, "contender.arg", []string{}, "add/override contender CLI flags") | ||
| recipeCmd.Flags().StringVar(&contenderTarget, "contender.target", "", "override the node that contender spams -- accepts names like \"el\"") | ||
| recipeCmd.Flags().StringArrayVar(&prefundedAccounts, "prefunded-account", []string{}, "Fund this account in addition to static prefunded accounts, the input should the account's private key in hexadecimal format prefixed with 0x, the account is added to L1 and to L2 (if present)") |
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.
Looks like we can provide multiple accounts here. So maybe the flag is add-prefunded-accounts or prefund-accounts and we can document that possibility ("fund these 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.
I aligned myself with the other array var &contenderArgs, "contender.arg" so no s.
| - `--log-level` (string): Log level to use (debug, info, warn, error, fatal). Defaults to `info`. | ||
| - `--labels` (key=val): Custom labels to apply to your deployment. | ||
| - `--disable-logs` (bool): Disable the logs for the services. Defaults to `false`. | ||
| - `--prefunded-account` (string, repeated): Fund this account in addition to static prefunded accounts, the input should the account's private key in hexadecimal format prefixed with 0x, the account is added to L1 and to L2 (if present). |
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.
Let's fix this depending on the flag if we adjust it based on the suggestion.
|
FYI it seems all default prefunded anvil accounts are also prefunded in builder-playground. |
| gen.Config.DepositContractAddress = gethcommon.HexToAddress(config.DepositContractAddress) | ||
|
|
||
| // add pre-funded accounts | ||
| prefundedBalance, _ := new(big.Int).SetString("10000000000000000000000", 16) |
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 is done like this to align with the other prefunded balances:
- https://github.com/flashbots/builder-playground/blob/main/playground/artifacts.go#L169
- https://github.com/flashbots/builder-playground/blob/main/playground/artifacts.go#L301
Do you want me to fully change all of them?
Usually if something is 1000 by default
I saw all numbers over the years, overly big doesn't change that much I would personally go with something bigger than smaller, just for those that don't we tiny amount of wei to perform their testing and input really big eth value.
Something like 99999999999999999 ETH.
All in all, let me know and I'll make the necessary work.
main.go
Outdated
| recipeCmd.Flags().BoolVar(&contenderEnabled, "contender", false, "spam nodes with contender") | ||
| recipeCmd.Flags().StringArrayVar(&contenderArgs, "contender.arg", []string{}, "add/override contender CLI flags") | ||
| recipeCmd.Flags().StringVar(&contenderTarget, "contender.target", "", "override the node that contender spams -- accepts names like \"el\"") | ||
| recipeCmd.Flags().StringArrayVar(&prefundedAccounts, "prefunded-account", []string{}, "Fund this account in addition to static prefunded accounts, the input should the account's private key in hexadecimal format prefixed with 0x, the account is added to L1 and to L2 (if present)") |
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 aligned myself with the other array var &contenderArgs, "contender.arg" so no s.
Add possibility to add more prefunded accounts to the L1 generated genesis by accepting a repeated
[]stringflag--prefunded-accountwhich accepts the same format (private key in hex) as the static prefunded accounts.I debated about putting the
prefunded-accountflag on the L1 recipe directly, to scope it just to this. Maybe it would be preferable, let me know what do you think.