feat: batching for reads and smart-account writes #773

d4mr · 2024-11-15T22:47:21Z

Test read with example curl:

curl --request POST \
  --url http://localhost:3005/contract/421614/read-batch \
  --header 'Authorization: Bearer {{authToken}}' \
  --header 'content-type: application/json' \
  --data '{
  "calls": [
    {
      "contractAddress": "0x75faf114eafb1BDbe2F0316DF893fd58CE46AA4d",
      "functionName": "name"
    },
    {
      "contractAddress": "0x75faf114eafb1BDbe2F0316DF893fd58CE46AA4d",
      "functionName": "totalSupply"
    }
  ]
}'

Test write cURL example:

curl --request POST \
  --url http://localhost:3005/backend-wallet/84532/send-transactions-atomic \
  --header 'Authorization: Bearer {{authToken}}' \
  --header 'Content-Type: application/json' \
  --header 'x-backend-wallet-address: <use a smart backend wallet> \
  --data '{
  "transactions": [
    {
      "toAddress": "0x5175E053bE7BDC74896C0D8675590FfbCbfe2E73",
      "value": "0",
      "data": "0xe5071b8e"
    },
    {
      "toAddress": "0x5175E053bE7BDC74896C0D8675590FfbCbfe2E73",
      "value": "0",
      "data": "0xe5071b8e"
    }
  ]
}

0xe5071b8e is the function selector for incrementCount(), and 0x5175E053bE7BDC74896C0D8675590FfbCbfe2E73 is deployed on base sepolia. On checking status of queued transaction, and viewing event logs for the sent transaction, logs from 0x51..73 should be visilble.

PR-Codex overview

This PR introduces support for batch operations in transaction processing, enhancing the ability to handle multiple transactions atomically. It includes new schemas, routes, and methods for batch reading and sending transactions, as well as updates to existing transaction handling.

Detailed summary

Added batchOperations field to various schemas and types.
Introduced sendTransactionBatchAtomic route for atomic transaction processing.
Created readBatchRoute for batch reading from multiple contracts.
Updated transaction handling to support batch operations.
Enhanced error handling for wallet details retrieval.
Modified OpenAPI URL in generate-sdk.ts for local development.
Implemented caching for wallet details to improve performance.

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

d4mr · 2024-11-15T22:50:56Z

changelog

src/server/routes/contract/read/read-batch.ts

github-actions · 2024-12-01T00:58:06Z

This PR is stale because it has been open for 7 days with no activity. Remove stale label or comment or this PR will be closed in 3 days.

zeet-co · 2025-01-13T04:31:10Z

We're building your pull request over on Zeet.
Click me for more info about your build and deployment.
Once built, this branch can be tested at: https://thirdweb-engine-ad8x-pb-batch-read.engine-aws-usw2.zeet.app before merging 😉

…t details retrieval

joaquim-verges · 2025-01-13T08:03:03Z

src/server/routes/backend-wallet/send-transactions-atomic.ts

+    Querystring: Static<typeof requestQuerystringSchema>;
+  }>({
+    method: "POST",
+    url: "/backend-wallet/:chain/send-transactions-atomic",


maybe send-batch-transaction ? we kinda use that language already in the sdk and docs

We already have a send-transaction-batch endpoint which just queues up all the transactions and sends them non atomically

Why don't you edit that endpoint then? Just do an actual batch if it's a smart backend wallet

I considered modifying the send-transaction-batch endpoint, but decided against it, because:

There's legitimate use-cases for non-atomic batching, even when using smart backend wallets. Batching atomically you might run into block gas limit, no limits on non-atomic batching though

Because of above reason, we shouldn't auto-force atomic batch on SBW. This might break user expectations, so instead having a batch?: boolean option would be good. This isn't easy to add, because existing endpoint takes an expects an array as the transaction body, no easy way to add to request body.
(We can still add to queryParam)

Another argument against automatic atomic batching when SBW: we also want to support atomic batching for regular EOA + Account Address style smart accounts.

There's also a couple of things don't make sense to share between the two batching strategies. Eg, idempotency-key. Non-atomic batching returns multiple queue-ids (queueIds: string[]), so an idempotency key doesn't work there, but it does make sense to include when using atomic batching, since there's only one queue-id returned.

shouldSimulate is not available as a query-param for non-atomic batch, due to latency of individually simulating transactions. But we can allow this for atomic batch.

As mentioned before: non atomic batch returns an array of queueIds, and atomic batch only returns a single queueId. If we automatically move to atomic batching on the same endpoint, and start returning only a single queueId in the array, client code that expects transactions.length == queueIds.length might break

Given the number of special cases to handle if we were to merge endpoints, I think having them separate is best.

You listed enough differences to justify a different endpoint rather than overloading one. 👍

More thoughts on names:

/send-ordered-transactions

/send-userop-batch

/send-transaction-sequence
I don't have a strong opinion against atomic, seems clear too.

ugh i see. is anyone actually using the non-atomic batch endpoint though? seems very niche when you can just send 2 api calls

joaquim-verges · 2025-01-13T08:09:36Z

src/server/routes/contract/read/read-batch.ts

+        const results = await readContract({
+          contract: multicall,
+          method: MULTICALL3_AGGREGATE_ABI,
+          params: [encodedCalls],


i think there was a limit to how much data you can pack in here, did you try a very large read?

There's definitely a limit, but failing and forwarding the error should be fine, no?

Yeah should be fine

arcoraven · 2025-01-13T08:02:31Z

src/server/routes/backend-wallet/send-transactions-atomic.ts

+    Querystring: Static<typeof requestQuerystringSchema>;
+  }>({
+    method: "POST",
+    url: "/backend-wallet/:chain/send-transactions-atomic",


There's already a /send-transactions-batch. IMO introducing /send-transactions-atomic will confuse the dev.

WDYT about reusing the same handler and branching the logic if SBW? In the docs we'd then need to clarify "Order is guaranteed for SBW only. If non-SBW, order is not guaranteed".

If you still want two separate paths, I'd prefer avoiding a new term (atomic) and using simpler language, like /send-transaction-batch-ordered. Or /send-userop-batch to make it clear it's for smart wallets.

answered why not the same handler in a question above, Joaquim had a similar question.

I do agree the name for the other path could be improved, but I ran out of ideas.

send-userop-batch could be misleading since we're creating a single UserOp rather than batching multiple UserOps.

I do like keeping "atomic" in the endpoint name since it's a widely understood term beyond web3 - developers regularly encounter it in databases, filesystems, and other contexts where operations need to succeed or fail as one unit, and it's more precise than "ordered".

I was initially thinking /send-transaction-batch-atomic to go along with the existing /send-transaction-batch, but felt it would be too long.

arcoraven · 2025-01-13T08:04:30Z

src/server/routes/backend-wallet/send-transactions-atomic.ts

+    Type.Object({
+      toAddress: Type.Optional(AddressSchema),
+      data: Type.String({
+        examples: ["0x..."],
+      }),
+      value: Type.String({
+        examples: ["10000000"],
+      }),
+    }),


Non-blocking: These are the minimal args of a "raw transaction" and may be useful to DRY since it's used in a few places. Then we can add descriptions too.

arcoraven · 2025-01-13T08:05:10Z

src/server/routes/backend-wallet/send-transactions-atomic.ts

+  transactions: Type.Array(
+    Type.Object({
+      toAddress: Type.Optional(AddressSchema),
+      data: Type.String({


Use HexSchema

arcoraven · 2025-01-13T08:08:32Z

src/server/routes/backend-wallet/send-transactions-atomic.ts

+      if (transactionRequests.length === 0) {
+        throw createCustomError(
+          "No transactions provided",
+          StatusCodes.BAD_REQUEST,
+          "NO_TRANSACTIONS_PROVIDED",
+        );
+      }


You should be able to assert this in TypeBox: { minItems: 1 }

arcoraven · 2025-01-13T08:09:18Z

src/server/routes/backend-wallet/send-transactions-atomic.ts

+      data: Type.String({
+        examples: ["0x..."],
+      }),
+      value: Type.String({


Use WeiAmountStringSchema

arcoraven · 2025-01-13T08:13:43Z

src/shared/db/wallets/get-wallet-details.ts

@@ -1,3 +1,4 @@
+import LruMap from "mnemonist/lru-map";


Nit: LRUMap to be consistent with other places we use this

arcoraven · 2025-01-13T08:17:24Z

src/server/routes/contract/read/read-batch.ts

+  Reply: Static<typeof responseSchema>;
+};
+
+export async function readMulticallRoute(fastify: FastifyInstance) {


Nit: I consider multicall the implementation, and any interface higher than this method shouldn't need to know about it. So var/fn names (even if not user facing) don't need to have multicall in them. Maybe batchReadRoute or readBatchRoute?

arcoraven · 2025-01-13T08:19:56Z

src/worker/tasks/send-transaction-worker.ts

+  if (queuedTransaction.batchOperations) {
+    queuedTransaction.batchOperations.map((op) => {
+      assert(op.to, "Invalid transaction parameters: to");
+    });
+  } else {
+    assert(toAddress, "Invalid transaction parameters: to");
+  }


For my understanding:

in a single userOp, toAddress is set.

in a batch userOp, toAddress is not set but it must be set on every batch operation.

What about contract deploys? Is there a toAddress on a userOp to deploy a contract?

for contract deployment, it should be 0x0000000000000000000000000000000000000000

I thought contract deployment to = null? Something to test

arcoraven · 2025-01-13T08:26:18Z

src/worker/tasks/send-transaction-worker.ts

+    const transactions = queuedTransaction.batchOperations
+      ? queuedTransaction.batchOperations.map((op) => ({
+          ...op,
+          chain,
+          client: thirdwebClient,
+        }))
+      : [
+          {
+            client: thirdwebClient,
+            chain,
+            ...queuedTransaction,
+            ...overrides,
+            to: getChecksumAddress(toAddress),
+          },
+        ];


We're explicitly ignoring overrides in the batch. That said, I recall discussion that gas/gasFee overrides for userOps don't make sense and we should remove them anyway. We should pick one or the other: this code does it both ways (overrides for single userOp, no overrides for batch userOps).

Non-blocking comment (maybe cleanup for later)

Single vs batch userOp flows should overlap as much as possible: there should be minimal difference other than the array of transactions being submitted.

That said I think all the "single vs batch userOp" logic should be in one place.

// pseudocode let transactions = [] if (batchOperations) { for (op of batchOperations) { assert(op.to) transactions.push({ ...op, chain, client }) } } else { transactions.push({ to, data, value, chain, client }) } // ...the rest of the code doesn't care about batchOperations

gas/gasFee overrides for userOps don't make sense

the current overrides don't work for userOps correct, but overrides in general can still be changed to use with userOps:

type UserOpGasOverrides = { callGasLimit?: bigint verificationGasLimit?: bigint preVerificationGas?: bigint maxFeePerGas?: bigint maxPriorityFeePerGas?: bigint }

even in the batch userop flow, there's only a single UserOp being constructed. And gas overrides are applied per UserOp, not per transaction inside the userop.

Refactor: Add readMulticall route for batch contract reads

adb4d58

bump thirdweb + serialise bigint

302f1f9

d4mr requested a review from arcoraven November 21, 2024 13:54

kien-ngo reviewed Nov 21, 2024

View reviewed changes