fix(common): kms correctly serializes transactions #2721

yonadaa · 2024-04-24T15:42:52Z

While testing the KMS deployer in #2704 I realised that it's transactions were rejected by the RPC because they were not serialised properly. This PR fixes the KMS account by more closely mirroring the Viem signTransaction logic.

changeset-bot · 2024-04-24T15:42:57Z

⚠️ No Changeset found

Latest commit: 664c49a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes changesets to release 24 packages

Name	Type
@latticexyz/common	Patch
@latticexyz/block-logs-stream	Patch
@latticexyz/cli	Patch
@latticexyz/config	Patch
@latticexyz/dev-tools	Patch
@latticexyz/faucet	Patch
@latticexyz/protocol-parser	Patch
@latticexyz/query	Patch
@latticexyz/store-indexer	Patch
@latticexyz/store-sync	Patch
@latticexyz/store	Patch
@latticexyz/world-modules	Patch
@latticexyz/world	Patch
mock-game-contracts	Patch
@latticexyz/react	Patch
@latticexyz/abi-ts	Patch
create-mud	Patch
@latticexyz/gas-report	Patch
@latticexyz/recs	Patch
@latticexyz/schema-type	Patch
@latticexyz/services	Patch
solhint-config-mud	Patch
solhint-plugin-mud	Patch
@latticexyz/utils	Patch

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

…saction

.changeset/slimy-emus-remember.md

holic · 2024-04-25T11:43:23Z

packages/common/src/account/kms/createKmsAccount.ts

+      // eslint-disable-next-line @typescript-eslint/explicit-function-return-type
+      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")
+          return {
+            ...transaction,
+            sidecars: false,
+          };
+        return transaction;
+      })();


ooc why this and not

Suggested change

// eslint-disable-next-line @typescript-eslint/explicit-function-return-type

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")

return {

...transaction,

sidecars: false,

};

return transaction;

})();

// 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

const signableTransaction = transaction.type === "eip4844" ? { ...transaction, sidecars: false } : transaction;

Copied verbatim from the Viem function! It could be more encapsulated imo: https://github.com/wevm/viem/blob/main/src/accounts/utils/signTransaction.ts#L53-L62

ahh got it!

maybe worth a note above about keeping this logic aligned with viem internals?

holic · 2024-04-25T11:44:00Z

packages/common/src/account/kms/createKmsAccount.ts

        address,
      });

-      return signature;
+      return serializer(transaction, signature);


~~just to confirm, we don't want to use signableTransaction here?~~

I see now we're following viem

Nope, again this is from viem. I think it's because the data you sign should (or shouldn't, idk) include some fields that are in the full transaction data.

holic · 2024-04-25T12:26:42Z

packages/common/src/account/kms/kmsKeyToAddress.ts

+import { signWithKms } from "./signWithKms";
+import { getAddressFromKms } from "./getAddressFromKms";
+
+export type kmsKeyToAccountOptions = {


Suggested change

export type kmsKeyToAccountOptions = {

export type KmsKeyToAccountOptions = {

holic · 2024-04-25T12:27:05Z

packages/common/src/exports/kms.ts

@@ -1 +1 @@
-export { createKmsAccount, type CreateKmsAccountOptions, type KMSAccount } from "../account/kms/createKmsAccount";
+export { kmsKeyToAccount, type kmsKeyToAccountOptions, type KMSAccount } from "../account/kms/kmsKeyToAddress";


Suggested change

export { kmsKeyToAccount, type kmsKeyToAccountOptions, type KMSAccount } from "../account/kms/kmsKeyToAddress";

export { KmsKeyToAccount, type kmsKeyToAccountOptions, type KmsAccount } from "../account/kms/kmsKeyToAddress";

holic · 2024-04-25T12:27:23Z

packages/common/src/account/kms/kmsKeyToAddress.ts

+  client?: KMSClient;
+};
+
+export type KMSAccount = LocalAccount<"aws-kms">;


Suggested change

export type KMSAccount = LocalAccount<"aws-kms">;

export type KmsAccount = LocalAccount<"aws-kms">;

holic · 2024-04-25T12:27:29Z

packages/common/src/account/kms/kmsKeyToAddress.ts

+export async function kmsKeyToAccount({
+  keyId,
+  client = new KMSClient(),
+}: kmsKeyToAccountOptions): Promise<KMSAccount> {


Suggested change

}: kmsKeyToAccountOptions): Promise<KMSAccount> {

}: kmsKeyToAccountOptions): Promise<KmsAccount> {

holic · 2024-04-25T12:30:10Z

packages/common/src/account/kms/kmsKeyToAddress.test.ts

+    let balance = await getBalance(adminClient, { address: account.address });
+    expect(balance).toEqual(0n);
+
+    let tx = await adminClient.sendTransaction({ to: account.address, value: parseEther("1") });
+    await waitForTransaction(tx);
+
+    balance = await getBalance(adminClient, { address: account.address });
+    expect(balance).toEqual(1000000000000000000n);


we're using a mix of tree shakable and not-tree shakable actions, would be nice to consolidate (ideally on tree shakable)

and if we're doing all tree shakable, might make sense to just use createClient not createWalletClient

holic · 2024-04-25T12:31:04Z

packages/common/src/account/kms/kmsKeyToAddress.test.ts

+    const adminClient = createWalletClient({
+      chain: foundry,
+      transport: http(anvilRpcUrl),
+      account: privateKeyToAccount("0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80"),


wanna make a note here about this being a default anvil user?

can also use testClient to set balance of ~any account

testClient would be more intuitive, the funding stuff is a means to an end

fix(common): kms correctly serializes transactions

8fa22c6

yonadaa requested review from alvrs and holic as code owners April 24, 2024 15:42

vercel bot deployed to Preview April 24, 2024 15:42 View deployment

chore: changeset

ed07da6

vercel bot deployed to Preview April 24, 2024 15:47 View deployment

holic previously approved these changes Apr 24, 2024

View reviewed changes

test: check kms account can sign transactions (#2723)

679c370

yonadaa dismissed holic’s stale review via 679c370 April 24, 2024 17:26

vercel bot deployed to Preview April 24, 2024 17:28 View deployment

yonadaa added 2 commits April 24, 2024 18:43

chore: different port

0a672cd

chore: change number

309176b

yonadaa requested a review from holic April 24, 2024 17:46

vercel bot deployed to Preview April 24, 2024 17:48 View deployment

Merge remote-tracking branch 'origin/main' into yonadaaa/fix-kms-tran…

3d60218

…saction

vercel bot deployed to Preview April 25, 2024 11:33 View deployment

yonadaa added 2 commits April 25, 2024 12:36

chore: delete changeset

f614769

fix: import file

70225d0

vercel bot deployed to Preview April 25, 2024 11:38 View deployment

vercel bot deployed to Preview April 25, 2024 11:40 View deployment

holic reviewed Apr 25, 2024

View reviewed changes

.changeset/slimy-emus-remember.md Outdated Show resolved Hide resolved

holic reviewed Apr 25, 2024

View reviewed changes

yonadaa added 2 commits April 25, 2024 12:50

docs: comment

3fbb101

refactor: kmsKeyToAccount

8a0b00f

vercel bot deployed to Preview April 25, 2024 11:52 View deployment

chore: changeset

a136a2f

vercel bot deployed to Preview April 25, 2024 11:55 View deployment

test: get transaction receipt

ab8487e

test: rearrange

618dd47

vercel bot deployed to Preview April 25, 2024 12:03 View deployment

yonadaa requested a review from holic April 25, 2024 12:15

holic reviewed Apr 25, 2024

View reviewed changes

refactor: rename types

eba9e6c

holic reviewed Apr 25, 2024

View reviewed changes

yonadaa added 2 commits April 25, 2024 13:32

test: refactor to use only actions

3b79f0c

test: use testClient, get tx receipt

6fc9ba6

vercel bot deployed to Preview April 25, 2024 12:38 View deployment

refactor: add getKeyId

664c49a

vercel bot deployed to Preview April 25, 2024 12:47 View deployment

yonadaa requested a review from holic April 25, 2024 12:48

holic approved these changes Apr 25, 2024

View reviewed changes

holic merged commit 182d706 into main Apr 25, 2024
12 checks passed

holic deleted the yonadaaa/fix-kms-transaction branch April 25, 2024 12:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(common): kms correctly serializes transactions #2721

fix(common): kms correctly serializes transactions #2721

yonadaa commented Apr 24, 2024 •

edited

Loading

changeset-bot bot commented Apr 24, 2024 •

edited

Loading

holic Apr 25, 2024

yonadaa Apr 25, 2024

holic Apr 25, 2024

holic Apr 25, 2024 •

edited

Loading

yonadaa Apr 25, 2024 •

edited

Loading

holic Apr 25, 2024

holic Apr 25, 2024

holic Apr 25, 2024

holic Apr 25, 2024

holic Apr 25, 2024

holic Apr 25, 2024

yonadaa Apr 25, 2024

	export type kmsKeyToAccountOptions = {
	export type KmsKeyToAccountOptions = {

		@@ -1 +1 @@
		export { createKmsAccount, type CreateKmsAccountOptions, type KMSAccount } from "../account/kms/createKmsAccount";
		export { kmsKeyToAccount, type kmsKeyToAccountOptions, type KMSAccount } from "../account/kms/kmsKeyToAddress";

	export type KMSAccount = LocalAccount<"aws-kms">;
	export type KmsAccount = LocalAccount<"aws-kms">;

	}: kmsKeyToAccountOptions): Promise<KMSAccount> {
	}: kmsKeyToAccountOptions): Promise<KmsAccount> {

fix(common): kms correctly serializes transactions #2721

fix(common): kms correctly serializes transactions #2721

Conversation

yonadaa commented Apr 24, 2024 • edited Loading

changeset-bot bot commented Apr 24, 2024 • edited Loading

⚠️ No Changeset found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holic Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

yonadaa Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonadaa commented Apr 24, 2024 •

edited

Loading

changeset-bot bot commented Apr 24, 2024 •

edited

Loading

holic Apr 25, 2024 •

edited

Loading

yonadaa Apr 25, 2024 •

edited

Loading