chore: track walletType usage #774
Conversation
| // Get wallet details. For EOA and SBW (v5 endpoints), `from` should return a valid backend wallet. | ||
| // For SBW (v4 endpoints), `accountAddress` should return a valid backend wallet. | ||
| // Else the provided details are incorrect (user error). | ||
| let walletDetails: ParsedWalletDetails | undefined; |
There was a problem hiding this comment.
This is the big change @d4mr. To track walletType we need to query walletDetails for all transactions (not just SBW).
|
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. |
| /** | ||
| * Try to resolve wallet details, determining if we're in V4 or V5 case | ||
| * For V5: wallet details should be found from 'from' address (Cases 1 & 3) | ||
| * For V4: wallet details are found from accountAddress (Cases 2 & 4) |
There was a problem hiding this comment.
This is kinda weird for case 4: v4 SDK for an EOA sender. This flow sets the accountAddress even if it's not a userOp?
| "Account not found", | ||
| StatusCodes.BAD_REQUEST, | ||
| "ACCOUNT_NOT_FOUND", | ||
| ); |
There was a problem hiding this comment.
Is there a clearer error here? Is this a use case the user can get into, or does it indicate a bug within Engine (5xx response)?
There was a problem hiding this comment.
if a wallet address that doesn't exist in engine at all is sent, then that's user error, so 4xx
| } | ||
|
|
||
| // Simulate the transaction. | ||
| // walletDetails is now narrowed to SmartBackendWalletDetails |
There was a problem hiding this comment.
We already know this is a smart backend wallet here.
Can we move validateSmartBackendWalletChainSupport out here? Then these transform methods are a lot simpler (and no longer async)
| return { processedTransaction }; | ||
| }; | ||
|
|
||
| const normalizeAddresses = ( |
There was a problem hiding this comment.
This abstraction feels unnecessary, can we move it back inline?
I want this file to mostly read linearly without having to jump into helper functions unless its super obvious what the function does (e.g. isSmartBackendWallet(...))
… and add walletType field
| } | ||
| // Handle idempotency | ||
| const queueId: string = idempotencyKey ?? randomUUID(); | ||
| if (idempotencyKey && (await TransactionDB.exists(queueId))) { |
There was a problem hiding this comment.
I see you're reducing this code for readability. Actually maybe this line can just be:
if (await TransactionDB.exists(queueId)) {The key difference being: if a RANDOMLY GENERATED queueId exists in DB (idempotency was not provided) this will fail. Previously, it would override the previous one. Arguably both are bad situations but extremely low probability, so might as well go with the simpler code.
| // from is set to the SBW account address (this should be the SBW signer address) | ||
| // these values need to be corrected so the worker can process the transaction | ||
| // Case 1 & 2: Wallet found by 'from' | ||
| if (walletFoundByFrom && walletDetails) { |
There was a problem hiding this comment.
Shouldn't these be the same? If walletDetails is truthy, walletFoundByFrom must be true here.
| walletDetails, | ||
| ); | ||
| } else { | ||
| // Case 2: Regular wallet (V4 or V5) - just add wallet type |
There was a problem hiding this comment.
Use "EOA" instead of "regular" for consistency and clarity
| // From this point on, we're in Case 3 territory - check for V4 smart backend wallet | ||
| if (!queuedTransaction.accountAddress) { | ||
| throw createCustomError( | ||
| "Account not found", |
There was a problem hiding this comment.
Is it possible for a user to get into this state? If so, we should make the error more clear.
Invalid backend wallet configuration. See: https://portal.thirdweb.com/engine/troubleshooting
And maybe put more help there.
If a user can't get here and this will only fail if there's a bug within Engine, this should just throw (treated as a 500).
There was a problem hiding this comment.
Same with the next two thrown errors below.
|
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. |
| }: GetWalletDetailsParams) => { | ||
| // Wallet details are stored in lowercase. | ||
| const walletAddress = _walletAddress.toLowerCase(); |
There was a problem hiding this comment.
ugh would love for all of us to align on lower vs checksum
|
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. |
closes infra-438
PR-Codex overview
This PR focuses on refactoring the
getWalletDetailsfunction to usewalletAddressinstead ofaddress, enhancing clarity and consistency across the codebase. Additionally, it introduces type improvements and cache handling for wallet details.Detailed summary
addresstowalletAddressingetWalletDetails.getWalletDetailsacross multiple files to use the new parameter.walletDetailsCachefor caching wallet details.walletTypeproperty in various transaction types.