Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

Ethermint key generation #78

Merged
merged 19 commits into from
Aug 11, 2019
Merged

Ethermint key generation #78

merged 19 commits into from
Aug 11, 2019

Conversation

austinabell
Copy link

Needed to replace the default key generation with within Cosmos because of the inability to change the underlying key signing and addressing structure and function implementations. I am making a PR now but this still isn't able to sign a genesis tx since the types cannot be used with gentx CLI commands since type is private within the overriden keys implementation. This PR allows the generation and functionality for Ethereum keys to be used either in our own genesis tx util or tx signing cli commands.

Other reason I split up is because it is not explicitly connected with the future functionality needed to sign and start the daemon and that can be decided how that should be functional. There is some code in these changes that may not completely transfer over to how we want it to be functional, including the main one which is how the address should be formatted and printed.

Currently I have it formatted to be the same format that underlying cosmos/tm node uses, which is something like: cosmos1pz5w72ncvn4swe3jedvufzakknr86l0fr259lq and is generated through:

    accAddr := sdk.AccAddress(info.GetPubKey().Address().Bytes())
    ...

	ko := KeyOutput{
		...
		Address: accAddr.String(),
		...
	}

	return ko, nil

but it can be changed back to the ethereum/hex format if that is how we want the genesis tx to work.

can be tested with the following commands:

make install
rm -rf ~/.emint*
emintcli emintkeys add austin
emintcli emintkeys show austin
emintcli emintkeys show austin -a

Which I included the show <key> -a because that is what is used when the daemon pulls the key info from storage to perform a signature with the key (or perform any of the functions with the key info)

I have kept the antehandler as it is for now because it hasn't been interacted with, but this is a step toward utilizing the ethermint antehandler #71

)

// Keybase exposes operations on a generic keystore
type Keybase interface {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any benefit to using the existing interface defined in the SDK? As opposed to re-defining it here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rationale was because the types of certain functions, including CreateAccount have different types as the ones declared in the base Cosmos SDK to achieve the functionality needed (I had to change the Info type). The types are intended to be completely separate from the standard key structs/interfaces but I had just used and shared functionality when possible to reduce the amount of duplicate code and functionality needed.

I know this doesn't come out very clean and I really meant to open this as a draft PR to find the cleanest solution without having to redeclare and rewrite all of this logic just to change the underlying key type and functionality.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon looking at it again, it appears as if it may be possible to revert the Keybase and Info interfaces to be based on the cosmos SDK ones, and I will check if this is the case tomorrow or remember why I had done this previously.

Copy link

@GregTheGreek GregTheGreek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review will finish later.

app/test_utils.go Show resolved Hide resolved
crypto/keys/codec.go Show resolved Hide resolved
@austinabell austinabell merged commit cfac906 into development Aug 11, 2019
@austinabell austinabell deleted the austin/emintkeys branch August 15, 2019 18:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants