Skip to content
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

feat(testutil): key generation should support both ed25519 and secp256k1. #1316

Closed
Unique-Divine opened this issue May 5, 2023 · 0 comments · Fixed by #1317
Closed

feat(testutil): key generation should support both ed25519 and secp256k1. #1316

Unique-Divine opened this issue May 5, 2023 · 0 comments · Fixed by #1317
Labels
type: enhancement New feature or request

Comments

@Unique-Divine
Copy link
Member

Unique-Divine commented May 5, 2023

Context

All of our key generation util functions in x/common/testutil are generated from ed25519. I found out when trying to import one of these accounts to the validator node's clientCtx.Keyring that support was never added on the binary for ed25519 keys.

Background

It seems the feature was de-prioritized and never implemented according to the comments in the SDK repo:

zmanian: "Disagree on Prelaunch. All fundraiser keys are secp keys. No one can even really use a new keypair until transfers are enabled."

ValarDragon: "That sounds good to me! This is definitely something that will be easier to support later. (Doesn't even involve changing the version) I think I'll just close this, and we can look into ed25519 again post launch / after hd derivation is suitably standardized."

Reproducibility

This was some of the code I wrote that made the error apparent:

import (
  // ...
  "github.com/cosmos/cosmos-sdk/crypto"
  cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
)

type Account struct {
	privKey    cryptotypes.PrivKey
	addr       sdk.AccAddress
	passphrase string
}

func (s *IntegrationSuite) AddRootToKeyring(root Account) {
	s.T().Log("add the x/sudo root account to the clientCtx.Keyring")
	// Encrypt the x/sudo root account's private key to get its "armor"
	passphrase := root.passphrase
	privKey := root.privKey
	armor := crypto.EncryptArmorPrivKey(privKey, passphrase, privKey.Type())
	// Import this account to the keyring
	val := s.network.Validators[0]
	s.NoError(
		val.ClientCtx.Keyring.ImportPrivKey("root", armor, passphrase),
	)
}

The root account was created using the PrivKeyAddresPairs function. The semantically relevant part of that function is:

func PrivKeyAddressPairs(n int) (keys []cryptotypes.PrivKey, addrs []sdk.AccAddress) {
    // ... {
    keys[i] = ed25519.GenPrivKeyFromSecret(secret)
    addrs[i] = sdk.AccAddress(keys[i].PubKey().Address())
  }
  return
}

I executed a transaction with the root as the sender and got the following output.

{
  "height": "0",
  "txhash": "58DBC804E118DC1D4BED13708BD2F3A15B41A61FF1828A40442DDA0BD1513DCA",
  "codespace": "sdk",
  "code": 8,
  "data": "",
  "raw_log": "ED25519 public keys are unsupported: invalid pubkey",
  "logs": [],
  "info": "",
  "gas_wanted": "200000",
  "gas_used": "53347",
  "tx": null,
  "timestamp": "",
  "events": []
}

Desired Solution

Both the testutil.AccAddress and testutil.PrivKeyAddressPairs functions in x/common/testutil should generate keys with secp256k1 instead of ed25519. We can replace or rename the current versions of these functions.

@Unique-Divine Unique-Divine added the type: enhancement New feature or request label May 5, 2023
matthiasmatt added a commit that referenced this issue May 9, 2023
…#1317)

* golang structs for the contract binding types

* feat(wasm): implement custom queries: Reserves, AllMarkets

* refactor: make request type names more obvious

* Wire in the WasmKeeper to the app with []wasm.Option set for the bindings

* add FundAccount fn to x/common/testutil

* rm fund.go. These functions were defined in the recent x/inflation PR

* refactor: test genesis setup to testutil/genesis so other modules can import th functions

* fix: add json serialization info to cw_query.go

* test(wasmbin): add perp bindings contract and add correctness tests

* test(wasm/binding): successful integration test using the real contract

* update CHANGELOG and include marshal.go

* fix: linter

* (wasm): impl and test base price query

* add utils

* impl and test BasePrice, Metrics, and PremiumFraction queries

* add remaining integration tests for the PR

* test,fix(wasm): all integration tests working on a real contract

* test,fix(wasm): all integration tests working on a real contract

* comment

* refactor(wasm): small rename to query_responses.json

* refactor,docs: PR comments

* test(cw_struct): test compatibility with rust binding types for execute msgs

* feat(wasm): add execute msg bindings for open pos, add & remove margin, close pos

* (wasm.go): wire the execute msg as a wasm.Option

* test(wasm): unit + integration tests for perp transactions

* changelog

* changelog

* test(wasm): check for position in state after it's open

* golangci-lint

* (wasm): new bindings contract

* feat(wasm): peg shift bindings + unit test + integration test

* changelog

* build: new contract with String types for sdk.Dec values that can be negative

* impl Position query with unit test

* test: integration test for Position query

* impl and test multiple position query

* test(wasm): verify correctness of fields in positions query

* checkpoint #wip sudo core business logic

* checkpoint #wip: implement all business logic with full test coverage

* test: add integration tests of the sudo contracts in x/wasm

* changelog

* changelog

* more docs

* refactor(sudo): run msg.ValidateBasic before the main handler

* refactor: small cleanup

* doc comment

* feat(sudo): cli commands + tests for tx and query

* feat(sample.go): secp256k1 usage for #1316

* chore: changelog + linter

* test(sudo): slice order can change when written into state

* test(sudo): fix boolean checks

* chore: linter

* chore: linter

* changelog

* remove ed25519Algo support in sample.go

---------

Co-authored-by: Matthias <97468149+matthiasmatt@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant