Add support for specifying per-key password #16
Conversation
kamsz
requested review from
ArielLaub,
RonTuretzky,
fbaralle and
uri-lightblocks
as code owners
|
|
||
| func RunDeregister(c *cli.Context) error { | ||
| passphrase := c.String(flag.PassphraseFlag.Name) | ||
| ecdsaPassphrase := c.String(flag.EcdsaPassphraseFlag.Name) |
There was a problem hiding this comment.
this should only override the general passphrase if the dedicated one is not provided, like the encrypt command works no?
There was a problem hiding this comment.
since deregister doesn't require more than one password i've left it as it is, but made it explicit with ecdsa passphrase flag that it is ecdsa key password
There was a problem hiding this comment.
we should be backward compatibility, so we should pass the two passphrase flags use the backward compatibility flag if the correct flag is empty
|
|
||
| func RunPrintStatus(c *cli.Context) error { | ||
| passphrase := c.String(flag.PassphraseFlag.Name) | ||
| ecdsaPassphrase := c.String(flag.EcdsaPassphraseFlag.Name) |
There was a problem hiding this comment.
same feedback to the deregister
kamsz
force-pushed
the
support-for-per-key-password
branch
from
ce047e6 to
022ef4c
Compare
| Action: runDeclareAlias, | ||
| Flags: []cli.Flag{ | ||
| flag.PassphraseFlag, | ||
| flag.EcdsaPassphraseFlag, |
There was a problem hiding this comment.
Declare alias requires the ecdsa private key to sign and submit the transaction. The alias ecdsa is used to get the address of the alias
| flag.EthRPCFlag, | ||
| flag.RegistryCoordinatorFlag, | ||
| flag.PassphraseFlag, | ||
| flag.EcdsaPassphraseFlag, |
There was a problem hiding this comment.
need to support the old PassphraseFlag (for backward compatibility)
| flag.EthRPCFlag, | ||
| flag.RegistryCoordinatorFlag, | ||
| flag.PassphraseFlag, | ||
| flag.EcdsaPassphraseFlag, |
There was a problem hiding this comment.
need to support the old PassphraseFlag (for backward compatibility)
| if passphrase == "" { | ||
| return cli.Exit("passphrase is required", 1) | ||
| if blsPassphrase == "" || ecdsaPassphrase == "" { | ||
| return cli.Exit("either passphrase or bls/ecdsa passphrase is required", 1) | ||
| } | ||
| } else { | ||
| blsPassphrase = passphrase | ||
| ecdsaPassphrase = passphrase | ||
| } |
There was a problem hiding this comment.
need to change the order of checking the flags.
first checks the new flags (the configuration we want to have)
second check the backward compatbility
| if passphrase == "" { | ||
| return cli.Exit("passphrase is required", 1) | ||
| if blsPassphrase == "" || ecdsaPassphrase == "" || aliasedEcdsaPassphrase == "" { | ||
| return cli.Exit("either passphrase or bls/ecdsa/aliased ecdsa passphrase is required", 1) | ||
| } | ||
| } else { | ||
| blsPassphrase = passphrase | ||
| ecdsaPassphrase = passphrase | ||
| aliasedEcdsaPassphrase = passphrase | ||
| } |
There was a problem hiding this comment.
same comment on checking first the new flags and later the backward compatibility
| if passphrase != "" { | ||
| blsPassphrase = passphrase | ||
| ecdsaPassphrase = passphrase | ||
| } |
There was a problem hiding this comment.
since there is default value to passphrase this will be true.
Need to reverse the order (or check if the new flags are empty)
| ecdsaPassphrase = passphrase | ||
| } | ||
|
|
||
| if blsPassphrase == "" || ecdsaPassphrase == "" || keyStorePath == "" { |
There was a problem hiding this comment.
with the new approach of having a passphrase per key, need to spilt this if into smaller and unrelated it statements
|
|
||
| func RunDeregister(c *cli.Context) error { | ||
| passphrase := c.String(flag.PassphraseFlag.Name) | ||
| ecdsaPassphrase := c.String(flag.EcdsaPassphraseFlag.Name) |
There was a problem hiding this comment.
we should be backward compatibility, so we should pass the two passphrase flags use the backward compatibility flag if the correct flag is empty
|
|
||
| func RunPrintStatus(c *cli.Context) error { | ||
| passphrase := c.String(flag.PassphraseFlag.Name) | ||
| ecdsaPassphrase := c.String(flag.EcdsaPassphraseFlag.Name) |
There was a problem hiding this comment.
same feedback to the deregister
| if passphrase == "" { | ||
| return cli.Exit("passphrase is required", 1) | ||
| if ecdsaPassphrase == "" || aliasedEcdsaPassphrase == "" { | ||
| return cli.Exit("either passphrase or ecdsa/aliased ecdsa passphrase is required", 1) | ||
| } | ||
| } else { | ||
| ecdsaPassphrase = passphrase | ||
| aliasedEcdsaPassphrase = passphrase |
There was a problem hiding this comment.
same comment on the order of the checks, first use the new flags
|
@kamsz Hey, is this still a pressing PR for your team? |
3 participants
This pull request adds support for specifying a different passwords for each key. It is useful if you plan to use a different password for BLS, ECDSA and aliased ECDSA keys.