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

ChainEndpoint trait makes restrictive assumptions about signing keys #3641

Open
5 tasks
hdevalence opened this issue Sep 29, 2023 · 1 comment
Open
5 tasks
Labels
E: non-cosmos External: related to non-Cosmos chains

Comments

@hdevalence
Copy link
Contributor

Summary

The current ChainEndpoint trait makes too restrictive assumptions about the shape and role of signing keys. This makes it difficult to properly add support for chains like Penumbra with different authorization behaviors.

Problem Definition

The ChainEndpoint trait used to create pluggable, chain-specific backends has a SigningKeyPair associated type.

This type must implement the SigningKeyPair trait, which demands the following behavior:

  • production of signatures on arbitrary byte slices;
  • generation of the account corresponding to that key pair;
  • a declaration of which supported key type it is (ed25519 or secp256k1), and the assumption that there is a pair of keys (private and public).

This is a layering violation, because it bakes in assumptions from the Cosmos SDK that may not apply to other chains. In fact, none of these properties obtain for Penumbra keys:

  • Penumbra signing keys are never used on arbitrary byte slices, because they do not sign the encoding of the transaction data. Instead, they sign over the transaction's effect hash, similar to Bitcoin's SIGHASH mechanism, and there is one signature per spend, rather than a single signer for the transaction. And each signature is produced with a fresh randomization of the verification key. There is no API to sign arbitrary byte slices. Instead, a complete TransactionPlan is passed to the custodian, who returns AuthorizationData used to build the transaction.

  • Penumbra signing keys control many (2^32) logical accounts simultaneously. Accounts are not disclosed to anyone other than the user who controls them. Each account has multiple addresses, a stable "default address" as well as 2^96 ephemeral addresses. Neither accounts nor addresses are disclosed by shielded transactions. Addresses (representing payment capability) are only disclosed by the user to desired counterparties, but are not published on-chain.

  • Penumbra's key heirarchy does not have a pair of keys. Instead it has a tree of keys, with variously attenuated capability: a spending key, representing spending capability, a viewing key, representing viewing capability, and many publicly unlinkable addresses, representing the capability to pay specific accounts controlled by that user's spending key.

However, although these assumptions are all violated by Penumbra's design, there is no real problem with relaying. There's just a different authorization mechanism. The only problem here arises from the layering violation of lifting chain-specific authorization mechanisms into the common ChainEndpoint trait.

Proposal

Transaction authorization should move out of the ChainEndpoint trait into the endpoint implementation itself.

Rather than demanding an opinionated KeyPair, the ChainEndpoint should only require the endpoint to provide the Signer field that should be used when building related packets.

Signing transactions should be an opaque and internal responsibility of the endpoint implementation.

Acceptance Criteria


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@romac
Copy link
Member

romac commented Oct 3, 2023

Thanks so much for taking the time to write this up 🙏

Overall, I fully agree with your assessment that exposing the underlying signing key type is a layering violation and prevents non-Cosmos chains from integrating with Hermes.

Your proposal sounds good as well, but before we can move forward with it, we need to figure out how to deal with the hermes keys command, which I guess would either:

a) need to be adapted to work with both Cosmos and Penumbra keys
b) be moved under a new cosmos subcommand, leaving it up to you to add a penumbra keys subcommand should you find the need for it
c) left as-is

Option (a) might be the most satisfying for both of us, provided m we manage to make it work for both types of chain without changing the interface of these commands too much.

Option (b) would be a breaking change of the CLIs, which we try to keep as infrequent as possible. But if (a) proves not to be doable or desirable, I am not completely moving everything Cosmos-specific under an aptly named subcommand for clarity now that we will have support for at least one non-Cosmos SDK chain.

Option (c) might be a good option in case you don't ever plan to add Penumbra-specific commands for managing keys.

What do you think?

@romac romac added the E: non-cosmos External: related to non-Cosmos chains label Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E: non-cosmos External: related to non-Cosmos chains
Projects
Status: 🩹 Triage
Development

No branches or pull requests

2 participants