Replace withAppKeyOrigin option with getAppKey
#35
Conversation
The new `getAppKeyring` method will return a new SimpleKeyring created with an app key derived from the provided origin. This serves as an alternative to optionally augmenting each wallet method with a set of options that may contain a `withAppKeyOrigin` value, which would be used to generate a new wallet on-demand. Moving app key generation to a single place makes it easier to limit access to sensitive information, such as the primary private keys. It also allows the consumer to use app keys more efficiently by using the same wallet for successive operations, rather than generating a new private key each time with `keccak`.
Gudahtt
force-pushed
the
get-app-keyring
branch
from
c42e7e8
to
427ff1a
Compare
Gudahtt
changed the title
withAppKeyOrigin option with getAppKeyingwithAppKeyOrigin option with getAppKey
|
I've updated this to return a private key instead of a keyring. The private key can easily be used to construct a keyring, or an |
|
I would prefer that publishing of v4.0.0 wait until this change has also been considered, as it also includes a breaking change. I'm hoping it'll be fairly straightforward to review. I haven't put it up yet because it conflicts with this PR a fair bit. |
| return reject(e) | ||
| } | ||
| }) | ||
| async getAppKey (address, origin) { |
There was a problem hiding this comment.
Thoughts on this method name? Would getPrivateAppKey or exportAppkey be better?
I was hoping that the "Key" part would suffice for communicating that this is a private key, but maybe not.
| const appKeyOriginBuffer = Buffer.from(origin, 'utf8') | ||
| const appKeyBuffer = Buffer.concat([privKey, appKeyOriginBuffer]) | ||
| const appKeyPrivKey = ethUtil.keccak(appKeyBuffer, 256) | ||
| return appKeyPrivKey |
There was a problem hiding this comment.
This currently returns a Buffer. Let me know if it'd be more appropriate to return a hex string instead, like exportAccount does.
There was a problem hiding this comment.
Yeah I'd love if we used buffers internally, but until we do, I lean towards being more consistent in the interface.
A private key is now returned instead of a keyring. This private app key is less restrictive, as it can be used to construct a SimpleKeyring or any other keyring/wallet the consumer might prefer to use.
Gudahtt
force-pushed
the
get-app-keyring
branch
from
db1135d
to
9b6cb6b
Compare
|
I'd be in favor of adding a |
|
I'm going to close this for now, pending further updates. |
The new
getAppKeyringmethod will return a new SimpleKeyring created with an app key derived from the provided origin. This serves as an alternative to optionally augmenting each wallet method with a set of options that may contain awithAppKeyOriginvalue, which would be used to generate a new wallet on-demand.Moving app key generation to a single place makes it easier to limit access to sensitive information, such as the primary private keys. It also allows the consumer to use app keys more efficiently by using the same wallet for successive operations, rather than generating a new private key each time with
keccak.Update: A private key is now returned instead of a keyring. This private app key is less restrictive, as it can be used to construct a SimpleKeyring or any other keyring/wallet the consumer might prefer to use.