ZkSync formatters #1361

speeddragon · 2023-10-17T11:46:05Z

This is a PR to add the formatters regarding discussion #1229 on adding Paymaster support for ZkSync chain.

I had some issues regarding the types, probably there are improvements to be made. Feel free to comment and let me know if I misunderstood something regarding the types/structure of Viem code.

I have the serializers implemented in another PR (#1388), but probably this one should be reviewed and merged before.

PR-Codex overview

Focus of the PR:

This PR adds ZkSync formatters and updates several files in the codebase.

Detailed summary:

Added ZkSync formatters.
Updated src/chains/utils/index.test.ts.
Updated src/utils/chain.ts.
Updated src/utils/formatters/formatter.ts.
Updated src/actions/public/simulateContract.ts.
Updated src/actions/wallet/writeContract.ts.
Updated src/actions/wallet/deployContract.ts.
Updated src/utils/formatters/extract.test.ts.
Updated src/actions/wallet/signTransaction.ts.
Updated src/actions/wallet/prepareTransactionRequest.ts.
Updated src/chains/utils/index.ts.
Updated src/actions/test/sendUnsignedTransaction.ts.
Updated src/actions/public/call.ts.
Updated src/actions/public/estimateGas.ts.
Updated src/actions/wallet/sendTransaction.ts.
Updated src/utils/formatters/extract.ts.
Updated src/chains/definitions/zkSync.ts.
Updated src/clients/decorators/public.ts.
Updated src/chains/definitions/zkSyncTestnet.ts.
Updated src/clients/decorators/wallet.ts.
Updated src/actions/wallet/signTransaction.ts.

The following files were skipped due to too many changes: src/clients/decorators/wallet.ts, src/chains/zksync/formatters.ts, src/chains/zksync/formatters.test-d.ts, src/chains/zksync/types.ts, src/chains/zksync/formatters.test.ts

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

vercel · 2023-10-17T11:46:10Z

Someone is attempting to deploy a commit to the wagmi Team on Vercel.

A member of the Team first needs to authorize it.

changeset-bot · 2023-10-17T11:46:10Z

🦋 Changeset detected

Latest commit: db5ac41

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
viem	Minor

Not sure what this means? Click here to learn what changesets are.

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

speeddragon · 2023-10-18T10:45:03Z

After trying to have a working example noticed that I need to change the transactionRequest to send the right information to the estimateGas call and probably the subsequent calls. I will be updating this PR.

jxom · 2023-10-20T01:01:09Z

I will be updating this PR.

Will move this to draft for now then!

Do you mind also giving me access to this branch? 🙏

speeddragon · 2023-10-20T01:38:58Z

To give permission I need to add you as a writer to this repo, right? I need to talk with someone to add seats to the organization because I don't have any left.

Regarding the status of this PR, I got this working with a PoC today, but the code isn't properly structured yet. There will be some changes that need to be applied to Viem to properly work with ZkSync, but I will submit and document them when I have a cleaner version.

speeddragon · 2023-10-24T18:56:07Z

src/chains/zksync/formatters.test-d.ts

+      Assign<
+        Partial<RpcTransactionReceipt>,
+        {


Had issues trying to understand why I couldn't get the same type in here because the logs property has some extra fields in the ZkSync case. Finally managed that with the Assign.

speeddragon · 2023-10-24T18:59:48Z

src/chains/zksync/formatters.ts

+    exclude: [
+      'paymaster',
+      'paymasterInput',
+      'customSignature',
+      'gasPerPubdata',
+      'factoryDeps',
+    ],


The default Viem code didn't allow me to specify these fields (only the ones from RpcTransactionRequest. This probably wouldn't affect the ZkSync integration, but I would prefer a clean body when interacting with JSON-RPC. By default, it just copies the args keys into the return type.

speeddragon · 2023-10-24T19:04:10Z

src/utils/formatters/extract.ts

 */
 export function extract(
  value: Record<string, unknown>,
  { format }: { format?: ChainFormatter['format'] },
 ) {
  if (!format) return {}
-  const keys = Object.keys(format({}))
+  const keys = Object.keys(value)


Not sure if this is the right way to do it, but with the previous code I couldn't access the custom values, because the way this is design is to provide the values that the format returns, and in my case the values are different.

For example, I have gasPerPubdata, paymaster, paymasterInput, etc. The return should be done with the following format:

eip712Meta: { gasPerPubdata: ..., paymasterParams: { paymaster: ..., paymasterInput: ... }, ... }

Another possibility is not to be so flexible and make sure the input comes with the eip712Meta field. Probably something to analyse in the EIP712 support I'll be working on.

This change would effectively make the function redundant. I've pushed a change to fix this so we still get the predicted behaviour (and still works for your case).

speeddragon · 2023-10-24T19:05:24Z

src/utils/formatters/formatter.ts

@@ -10,7 +10,7 @@ export function defineFormatter<TType extends string, TParameters, TReturnType>(
  return <
    TOverrideParameters,
    TOverrideReturnType,
-    TExclude extends (keyof TParameters)[] = [],
+    TExclude extends (keyof (TParameters & TOverrideParameters))[] = [],


To be able to exclude gasPerPubdata and the other specific fields from transactionReceipt, I needed to add TOverrideParams to not get the type error. I think it makes sense, but would love to get some feedback.

jxom

PR is looking really solid! Good start. Just a few comments!

jxom · 2023-10-24T23:34:47Z

src/chains/zksync/types.ts

+// Transaction Request
+// https://era.zksync.io/docs/reference/concepts/transactions.html
+
+export type ZkSyncTransactionRequest = TransactionRequest &


Are we still meant to support the base txn types still here (ie. eip1559, eip2930, etc)? This structure assumes that the only supported request type is "eip712", but I'd assume it's not right? Should we make a ZkSyncTransactionRequestEIP712 type and unionize that onto here like we do for Celo's CIP42?

e.g.

export type ZkSyncTransactionRequest = | TransactionRequest | TransactionRequestEIP712

You're right. During my development process trying to understand the types, I had several iterations. I will add tests to show the support of normal transactions and fix the types. Thanks.

Ok, I know why I removed it. With this type, the type system can only see the common types and will complain about the fields on the exclude.

A workaround is to do like this:

export type ZkSyncTransactionRequest = // Not sure why I need to add the fields with never, but exclude complains if not added. | (TransactionRequest & { paymaster: never paymasterInput: never gasPerPubdata: never customSignature: never factoryDeps: never }) | ZkSyncTransactionRequestEIP712

But this will complain in other places as well. Not sure how to handle this. Currently the type issues are:

formatters.exs, complains about exclude params.

formatters.test-d.exs, complains about transactionRequest argument type and return type. I've checked the excludes and extract from vitest but couldn't make this work.

jxom · 2023-10-24T23:38:14Z

src/chains/zksync/types.ts

+  logs: RpcLog[]
+  l2ToL1Logs: RpcL2ToL1Log[]
+  // Why root isn't added into RpcTransactionReceipt?
+  root: Hex


Could add it in, but I believe only transactions that include root are ones before the Byzantium upgrade (2017)? I guess it would be different case for ZkSync though!

Yes, looks like it! https://docs.ethers.org/v5/api/providers/types/#providers-TransactionReceipt
I will check if there is a special case for ZkSync to use it or not. If not, maybe this should be added to the RpcTransactionReceipt as an optional?

jxom · 2023-10-24T23:40:06Z

src/chains/zksync/formatters.test.ts

Would be nice to see some tests for transaction requests here, and generally have coverage on uncovered lines. Can run coverage with bun run test:cov zksync/formatters.test.ts.

Thanks, I will check that. Had some issues trying to understand how should I run the tests to make sure I didn't break any previous behaviour but I think I have a lot of tests not being run but not sure why. Some of them failed with chainId defined as undefined but received 1.

Coverage should be 100% now.

jxom · 2023-10-25T00:04:00Z

src/utils/formatters/extract.ts

 */
 export function extract(
  value: Record<string, unknown>,
  { format }: { format?: ChainFormatter['format'] },
 ) {
  if (!format) return {}
-  const keys = Object.keys(format({}))
+  const keys = Object.keys(value)


This change would effectively make the function redundant. I've pushed a change to fix this so we still get the predicted behaviour (and still works for your case).

jxom · 2023-10-25T21:34:39Z

Just pushed a few more tweaks! Make sense to merge once #1388 is ready?

speeddragon · 2023-10-25T21:43:23Z

Just pushed a few more tweaks! Make sense to merge once #1388 is ready?

Thanks for the fixes! Yap, I'm cleaning up the serializer and will push the final version.

…some weird behaviour. Having it support null it won't complain in the browser side

speeddragon · 2023-10-26T16:42:20Z

Updated the code with some handling of null fields, one that can occur on a successful transaction (log.l1BatchNumber) and another when the nonce is wrong.

The last test in the formatter it complaining because of the extra fields that come from the args, not sure if the exclude should be kept in the formatter or not. This will not have any impact on the request methods (estimateGas, etc), so I can update the test to allow them and make it green.

* ZkSync formatters * Update formatters and types * Remove serializer code * refactor * fix * Update types, improve code coverage * Fix types * refactor * refactor * types * Support null in some fields, because when nonce is wrong can lead to some weird behaviour. Having it support null it won't complain in the browser side * fix * Create tiny-dolls-look.md --------- Co-authored-by: David Magalhães <[email protected]> Co-authored-by: moxey.eth <[email protected]>

ZkSync formatters

c0764a2

jxom marked this pull request as draft October 20, 2023 01:01

David Magalhães added 2 commits October 24, 2023 16:57

Merge branch 'main' into zksync_formatters

a96ab86

Update formatters and types

7cec75e

speeddragon commented Oct 24, 2023

View reviewed changes

Remove serializer code

29ba2db

speeddragon marked this pull request as ready for review October 24, 2023 19:36

speeddragon mentioned this pull request Oct 24, 2023

Add ZkSync EIP712 Serializers #1388

Merged

jxom force-pushed the zksync_formatters branch from bd3b806 to 29ba2db Compare October 24, 2023 23:46

refactor

3d374a3

jxom reviewed Oct 25, 2023

View reviewed changes

jxom and others added 7 commits October 25, 2023 12:21

fix

1c59acd

Merge branch 'main' into zksync_formatters

c2b1587

Update types, improve code coverage

b09a47f

Fix types

610f7b0

refactor

364ef5e

refactor

bce930f

types

224f2b2

Support null in some fields, because when nonce is wrong can lead to …

b89b4fb

…some weird behaviour. Having it support null it won't complain in the browser side

jxom added 2 commits October 27, 2023 07:53

fix

b0c0fb2

Create tiny-dolls-look.md

db5ac41

jxom merged commit 7059301 into wevm:main Oct 26, 2023
1 check failed

jxom deleted the zksync_formatters branch October 26, 2023 20:55

github-actions bot mentioned this pull request Oct 26, 2023

chore: version package #1401

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZkSync formatters #1361

ZkSync formatters #1361

speeddragon commented Oct 17, 2023 •

edited by pr-codex bot

Loading

vercel bot commented Oct 17, 2023

changeset-bot bot commented Oct 17, 2023 •

edited

Loading

speeddragon commented Oct 18, 2023

jxom commented Oct 20, 2023

speeddragon commented Oct 20, 2023

speeddragon Oct 24, 2023

speeddragon Oct 24, 2023

speeddragon Oct 24, 2023

jxom Oct 25, 2023

speeddragon Oct 24, 2023

jxom left a comment

jxom Oct 24, 2023

speeddragon Oct 25, 2023

speeddragon Oct 25, 2023

jxom Oct 24, 2023

speeddragon Oct 25, 2023 •

edited

Loading

jxom Oct 24, 2023

speeddragon Oct 25, 2023

speeddragon Oct 25, 2023

jxom Oct 25, 2023

jxom commented Oct 25, 2023

speeddragon commented Oct 25, 2023

speeddragon commented Oct 26, 2023 •

edited

Loading

ZkSync formatters #1361

ZkSync formatters #1361

Conversation

speeddragon commented Oct 17, 2023 • edited by pr-codex bot Loading

PR-Codex overview

Focus of the PR:

Detailed summary:

vercel bot commented Oct 17, 2023

changeset-bot bot commented Oct 17, 2023 • edited Loading

🦋 Changeset detected

speeddragon commented Oct 18, 2023

jxom commented Oct 20, 2023

speeddragon commented Oct 20, 2023

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

jxom left a 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

speeddragon Oct 25, 2023 • 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

jxom commented Oct 25, 2023

speeddragon commented Oct 25, 2023

speeddragon commented Oct 26, 2023 • edited Loading

speeddragon commented Oct 17, 2023 •

edited by pr-codex bot

Loading

changeset-bot bot commented Oct 17, 2023 •

edited

Loading

speeddragon Oct 25, 2023 •

edited

Loading

speeddragon commented Oct 26, 2023 •

edited

Loading