-
Notifications
You must be signed in to change notification settings - Fork 91
feat: batching for reads and smart-account writes #773
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
Changes from 7 commits
adb4d58
302f1f9
b818ef2
e56086c
82b8ce6
f24e84b
1904ce5
215c458
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,151 @@ | ||
import { Type, type Static } from "@sinclair/typebox"; | ||
import type { FastifyInstance } from "fastify"; | ||
import { StatusCodes } from "http-status-codes"; | ||
import type { Address, Hex } from "thirdweb"; | ||
import { insertTransaction } from "../../../shared/utils/transaction/insert-transaction"; | ||
import { AddressSchema } from "../../schemas/address"; | ||
import { | ||
requestQuerystringSchema, | ||
standardResponseSchema, | ||
transactionWritesResponseSchema, | ||
} from "../../schemas/shared-api-schemas"; | ||
import { | ||
maybeAddress, | ||
walletChainParamSchema, | ||
walletWithAAHeaderSchema, | ||
} from "../../schemas/wallet"; | ||
import { getChainIdFromChain } from "../../utils/chain"; | ||
import { | ||
getWalletDetails, | ||
isSmartBackendWallet, | ||
type ParsedWalletDetails, | ||
WalletDetailsError, | ||
} from "../../../shared/db/wallets/get-wallet-details"; | ||
import { createCustomError } from "../../middleware/error"; | ||
|
||
const requestBodySchema = Type.Object({ | ||
transactions: Type.Array( | ||
Type.Object({ | ||
toAddress: Type.Optional(AddressSchema), | ||
data: Type.String({ | ||
examples: ["0x..."], | ||
}), | ||
value: Type.String({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
examples: ["10000000"], | ||
}), | ||
}), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
), | ||
}); | ||
|
||
export async function sendTransactionsAtomicRoute(fastify: FastifyInstance) { | ||
fastify.route<{ | ||
Params: Static<typeof walletChainParamSchema>; | ||
Body: Static<typeof requestBodySchema>; | ||
Reply: Static<typeof transactionWritesResponseSchema>; | ||
Querystring: Static<typeof requestQuerystringSchema>; | ||
}>({ | ||
method: "POST", | ||
url: "/backend-wallet/:chain/send-transactions-atomic", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's already a 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe send-batch-transaction ? we kinda use that language already in the sdk and docs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already have a send-transaction-batch endpoint which just queues up all the transactions and sends them non atomically There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't you edit that endpoint then? Just do an actual batch if it's a smart backend wallet There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I considered modifying the
Given the number of special cases to handle if we were to merge endpoints, I think having them separate is best. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You listed enough differences to justify a different endpoint rather than overloading one. 👍 More thoughts on names:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ugh i see. is anyone actually using the non-atomic batch endpoint though? seems very niche when you can just send 2 api calls |
||
schema: { | ||
summary: "Send a batch of raw transactions atomically", | ||
description: | ||
"Send a batch of raw transactions in a single UserOp. Can only be used with smart wallets.", | ||
tags: ["Backend Wallet"], | ||
operationId: "sendTransactionsAtomic", | ||
params: walletChainParamSchema, | ||
body: requestBodySchema, | ||
headers: Type.Omit(walletWithAAHeaderSchema, ["x-transaction-mode"]), | ||
querystring: requestQuerystringSchema, | ||
response: { | ||
...standardResponseSchema, | ||
[StatusCodes.OK]: transactionWritesResponseSchema, | ||
}, | ||
}, | ||
handler: async (request, reply) => { | ||
const { chain } = request.params; | ||
const { | ||
"x-backend-wallet-address": fromAddress, | ||
"x-idempotency-key": idempotencyKey, | ||
"x-account-address": accountAddress, | ||
"x-account-factory-address": accountFactoryAddress, | ||
"x-account-salt": accountSalt, | ||
} = request.headers as Static<typeof walletWithAAHeaderSchema>; | ||
const chainId = await getChainIdFromChain(chain); | ||
const shouldSimulate = request.query.simulateTx ?? false; | ||
const transactionRequests = request.body.transactions; | ||
|
||
const hasSmartHeaders = !!accountAddress; | ||
|
||
// check that we either use SBW, or send using EOA with smart wallet headers | ||
if (!hasSmartHeaders) { | ||
let backendWallet: ParsedWalletDetails | undefined; | ||
|
||
try { | ||
backendWallet = await getWalletDetails({ | ||
address: fromAddress, | ||
}); | ||
} catch (e: unknown) { | ||
if (e instanceof WalletDetailsError) { | ||
throw createCustomError( | ||
`Failed to get wallet details for backend wallet ${fromAddress}. ${e.message}`, | ||
StatusCodes.BAD_REQUEST, | ||
"WALLET_DETAILS_ERROR", | ||
); | ||
} | ||
} | ||
|
||
if (!backendWallet) { | ||
throw createCustomError( | ||
"Failed to get wallet details for backend wallet. See: https://portal.thirdweb.com/engine/troubleshooting", | ||
StatusCodes.INTERNAL_SERVER_ERROR, | ||
"WALLET_DETAILS_ERROR", | ||
); | ||
} | ||
|
||
if (!isSmartBackendWallet(backendWallet)) { | ||
throw createCustomError( | ||
"Backend wallet is not a smart wallet, and x-account-address is not provided. Either use a smart backend wallet or provide x-account-address. This endpoint can only be used with smart wallets.", | ||
StatusCodes.BAD_REQUEST, | ||
"BACKEND_WALLET_NOT_SMART", | ||
); | ||
} | ||
} | ||
|
||
if (transactionRequests.length === 0) { | ||
throw createCustomError( | ||
"No transactions provided", | ||
StatusCodes.BAD_REQUEST, | ||
"NO_TRANSACTIONS_PROVIDED", | ||
); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should be able to assert this in TypeBox: |
||
|
||
const queueId = await insertTransaction({ | ||
insertedTransaction: { | ||
transactionMode: undefined, | ||
isUserOp: false, | ||
chainId, | ||
from: fromAddress as Address, | ||
accountAddress: maybeAddress(accountAddress, "x-account-address"), | ||
accountFactoryAddress: maybeAddress( | ||
accountFactoryAddress, | ||
"x-account-factory-address", | ||
), | ||
accountSalt: accountSalt, | ||
batchOperations: transactionRequests.map((transactionRequest) => ({ | ||
to: transactionRequest.toAddress as Address | undefined, | ||
data: transactionRequest.data as Hex, | ||
value: BigInt(transactionRequest.value), | ||
})), | ||
}, | ||
shouldSimulate, | ||
idempotencyKey, | ||
}); | ||
|
||
reply.status(StatusCodes.OK).send({ | ||
result: { | ||
queueId, | ||
}, | ||
}); | ||
}, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
HexSchema