Secp v3 keystore files bulk loading in eth1 mode #892
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
-- Add cli options -- Add SecpWalletBulkLoader and test -- Refactor some common code from BlsBulkloader
usmansaleem
added
the
doc-change-required
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
signing/src/main/java/tech/pegasys/web3signer/signing/bulkloading/SecpWalletBulkloader.java
Fixed
Show fixed
Hide fixed
signing/src/main/java/tech/pegasys/web3signer/signing/bulkloading/SecpWalletBulkloader.java
Fixed
Show fixed
Hide fixed
usmansaleem
requested review from
gfukushima,
siladu and
jframe
and removed request for
gfukushima and
siladu
siladu
reviewed
Aug 31, 2023
core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.java
Outdated
Show resolved
Hide resolved
...src/main/java/tech/pegasys/web3signer/commandline/config/PicoV3WalletBulkloadParameters.java
Outdated
Show resolved
Hide resolved
|
|
||
| import picocli.CommandLine.Option; | ||
|
|
||
| public class PicoV3WalletBulkloadParameters implements KeystoresParameters { |
There was a problem hiding this comment.
I think if we want to regard wallets as a type of keystore then the CLI arg should reflect that and also be called --keystores-path. That would mean it's the mode that determines the reading of the keystore, rather than the option name.
There was a problem hiding this comment.
Agree, think this should be --keystores-path
There was a problem hiding this comment.
done. Modified various instance variables/class names to use v3Keystores as well (instead of wallet).
commandline/src/main/java/tech/pegasys/web3signer/commandline/subcommands/Eth1SubCommand.java
Outdated
Show resolved
Hide resolved
acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/utils/HealthCheckResultUtil.java
Outdated
Show resolved
Hide resolved
commandline/src/main/java/tech/pegasys/web3signer/commandline/subcommands/Eth1SubCommand.java
Outdated
Show resolved
Hide resolved
signing/src/main/java/tech/pegasys/web3signer/signing/bulkloading/SecpWalletBulkloader.java
Outdated
Show resolved
Hide resolved
signing/src/main/java/tech/pegasys/web3signer/signing/bulkloading/SecpWalletBulkloader.java
Outdated
Show resolved
Hide resolved
jframe
reviewed
Sep 1, 2023
|
|
||
| final String jsonBody = healthcheckResponse.body().asString(); | ||
| final int keysLoaded = | ||
| getHealthCheckKeysCheckData(jsonBody, KEYS_CHECK_V3_WALLET_BULK_LOADING, "keys-loaded"); |
There was a problem hiding this comment.
Can this getHealthCheckKeysCheckData be also used in the bulk loading ATs
There was a problem hiding this comment.
I've refactored other places where healthcheck was used.
| final String jsonBody = healthcheckResponse.body().asString(); | ||
| final int keysLoaded = | ||
| getHealthCheckKeysCheckData(jsonBody, KEYS_CHECK_V3_WALLET_BULK_LOADING, "keys-loaded"); | ||
| assertThat(keysLoaded).isEqualTo(publicKeys.size()); |
There was a problem hiding this comment.
can the error count also be checked as well?
|
|
||
| import picocli.CommandLine.Option; | ||
|
|
||
| public class PicoV3WalletBulkloadParameters implements KeystoresParameters { |
There was a problem hiding this comment.
Agree, think this should be --keystores-path
signing/src/main/java/tech/pegasys/web3signer/signing/bulkloading/BlsKeystoreBulkLoader.java
Show resolved
Hide resolved
...ing/src/main/java/tech/pegasys/web3signer/signing/bulkloading/SecpV3KeystoresBulkLoader.java
Dismissed
Show dismissed
Hide dismissed
usmansaleem
changed the title
jframe
approved these changes
Sep 4, 2023
bgravenorst
removed
the
doc-change-required
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Description
SECP wallet files bulk loading in
eth1mode. Introduced following cli options:--keystores-pathThe path to a directory storing v3 wallet files. Wallet files must use a .json file extension.--keystores-passwords-pathThe path to a directory with the corresponding password files for the wallet files. Filename must match the corresponding wallet filename but with a .txt extension. This cannot be set if --keystores-password-file is also specified.--keystores-password-fileThe path to a file that contains the password that all wallets use. This cannot be set if --keystores-passwords-path is also specified.Summary of code changes:
commandline/src/main/java/tech/pegasys/web3signer/commandline/config/PicoV3KeystoresBulkloadParameters.javaV3 Keystores bulkloading CLI options. Implements
KeystoresParametersinterface which enforces same method names as BLS Bulk loading in ETH2 mode. The description for each parameter is different.commandline/src/main/java/tech/pegasys/web3signer/commandline/subcommands/Eth1SubCommand.javaMixin and validation of cli options. Either password directory or password file can be specified (not both).
core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.javabulkLoadSignersmethod is modified to wireSecpWalletBulkloader.signing/src/main/java/tech/pegasys/web3signer/signing/bulkloading/BlsKeystoreBulkLoader.javaCommon code has been extracted.
signing/src/main/java/tech/pegasys/web3signer/signing/bulkloading/SecpV3KeystoresBulkLoader.javaLoad v3 wallet files from specified directory by using either password file or corresponding password files from a directory.
acceptance-tests/src/test/java/tech/pegasys/web3signer/dslAdded acceptance test that covers using password path (dir), password file with direct cli options and using config file.
Fixed Issue(s)
Fixes #833
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog
Testing