feat: viem account from gcp hsm #2
Conversation
jeanregisser
force-pushed
the
jeanregisser/viem-hsm-gcp
branch
from
df67464
to
c7e850a
Compare
src/index.ts
Outdated
| * | ||
| * https://www.ssl.com/guide/pem-der-crt-and-cer-x-509-encodings-and-conversions/#:~:text=DER%20(Distinguished%20Encoding%20Rules)%20is,commonly%20seen%20in%20Java%20contexts. | ||
| */ | ||
| function pemToDerEncode(pem: string): string { |
There was a problem hiding this comment.
So, the KMS client returns the typical human-readable (PEM) public key that we're used to dealing with - this function removes the begin/end statements and trims whitespace, resulting in the base64 representation of the DER format key (the only difference between PEM and DER are presumably the comments present in PEM files). Taking a buffer of the base64 DER key presumably gives you the "canonical" (read: binary) representation of DER keys.
By my understanding, ASN.1 refers to the abstract syntax tree of the underlying data, not its encoding scheme (both PEM and DER are encodings of ASN.1 data, I believe; this article helped clarify). Given that, the name of the function publicKeyFromAsn1 is a bit confusing, since what's really important is the encoding scheme used for the input argument, which in this case is the binary DER encoding. I can't tell from publicKeyFromAsn1 whether the input should be PEM, binary DER, or b64 encoded. Maybe publicKeyFromDer might be more useful?
There was a problem hiding this comment.
oh I see, I didn't put much thought to this as this was the name used in the implementation I ported.
Renaming now, makes more sense, thanks.
src/index.ts
Outdated
| return pem.split('\n').slice(1, -2).join('').trim() | ||
| } | ||
|
|
||
| function publicKeyFromAsn1(bytes: Uint8Array): Hex { |
There was a problem hiding this comment.
What format exactly is this resulting public key?
There was a problem hiding this comment.
Ah, okay - this is ECDSA public key (just a long hex string), and the eth address is just the hash of the first chunk of it?
| const signableTransaction = (() => { | ||
| // For EIP-4844 Transactions, we want to sign the transaction payload body (tx_payload_body) without the sidecars (ie. without the network wrapper). | ||
| // See: https://github.com/ethereum/EIPs/blob/e00f4daa66bd56e2dbd5f1d36d09fd613811a48b/EIPS/eip-4844.md#networking | ||
| if (transaction.type === 'eip4844') |
There was a problem hiding this comment.
i can't say i fully understand this, but makes me wonder if there are other transaction types that we need to care about/do special stuff for here? if someone tries to use this for some new TX type from some new EIP down the line that requires special treatment, it seems like this wouldn't necessarily work? i guess there's not much to do about that besides allow-listing a subset of transaction types, but that definitely seems like overkill.
There was a problem hiding this comment.
Yes me neither, it's a bit odd it's at this level.
I copied what was done in viem: https://github.com/wevm/viem/blob/e6c47807f32d14ded53c40831177ee80c5a47a10/src/accounts/utils/signTransaction.ts
It feels like this should be put somewhere else in viem so each custom Account implementation doesn't have to deal with this.
I'll give this feedback to the viem authors.
Yes same, it looks small and simple. |
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Full working implementation with support for signing transactions, messages and typed data.
Ported from https://github.com/celo-org/developer-tooling/tree/0c61e7e02c741fe10ecd1d733a33692d324cdc82/packages/sdk/wallets/wallet-hsm-gcp
Send script run
Next up: add e2e test for CI, README, automated publish to NPM
Fixes RET-1037