Support transaction-introspection for more RPC responses - #1814
Conversation
🦋 Changeset detectedLatest commit: bec07ce The changes in this PR will be included in the next version bump. This PR includes changesets to release 48 packages
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 |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
BundleMonFiles updated (7)
Unchanged files (143)
Total files change +174B +0.03% Final result: ✅ View report in BundleMon website ➡️ |
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Generalises decodeTransactionFromRpcResponse so it no longer takes method-specific GetTransaction* types. Instead it takes two structural shapes (DecodableWireTransactionResponse / DecodableJsonTransactionResponse) that model only the transaction / meta / version fields actually read. That's a clean refactor: it decouples the decoder from @solana/rpc-api, lets getBlock and getTransactionsForAddress results flow through unchanged, and simplifies the overload set from three type-parameter-heavy signatures to two straightforward ones.
The wire-vs-JSON split on the shape of transaction (base-encoded tuple vs. { message: … }) keeps overload resolution unambiguous per encoding. Tightening the JSON return to transaction?: never is a nice bit of extra rigour — it was previously typed as an optional Transaction, which the JSON path never actually produces.
Good catch on rpcTx.version ?? 'legacy' vs. the old 'version' in rpcTx check: with version now a declared optional key on the input type, the in check would return true even when the property is undefined. The ?? (not ||) note in the comment is right — 0 is a legitimate version.
The new runtime test that includes slot / blockTime / transactionIndex on the input object is a good addition. And deriving the typetest inputs from the real GetTransactionsForAddressApi / GetBlockApi result types (rather than hand-written literals) is the right call to avoid excess-property-check false negatives.
Things to watch out for
1. @solana/rpc-types should probably move to dependencies. The source now imports Base58EncodedBytes, Base58EncodedDataResponse, Base64EncodedDataResponse, and Blockhash at the type level, and those references leak into the emitted .d.ts via the exported overload signatures (through DecodableWireTransactionResponse / DecodableJsonTransactionResponse). It's currently in devDependencies only. Compare @solana/transaction-messages/package.json, which lists @solana/rpc-types as a runtime dependency for the same reason. Consumers of @solana/transaction-introspection who happen to not already have @solana/rpc-types installed would get a broken types resolution.
2. Conversely, @solana/rpc-api may no longer belong in dependencies. After this PR, the source file no longer imports anything from @solana/rpc-api — only the tests and typetests do. Consider demoting it to devDependencies. (Worth double-checking the rest of the package's src/ first — I only inspected the file changed here.)
3. getBlock with encoding: 'json' + maxSupportedTransactionVersion is not covered by the typetest, and I suspect it wouldn't type-check today. TransactionForFullTransactionAddressTableLookups in @solana/rpc-types declares addressTableLookups?: readonly AddressTableLookup[] | null (nullable), but DecodableJsonTransactionResponse.message.addressTableLookups? is not nullable. So rpc.getBlock(slot, { encoding: 'json', maxSupportedTransactionVersion: 0, transactionDetails: 'full' }).transactions[i] should be rejected by the overload as an argument. The changeset text advertises getBlock results decoding, so this gap is worth either closing (widen the field to | null and let the existing message.addressTableLookups ? ... : [] guard handle null — which it already does, since null is falsy) or explicitly documenting. Adding a typetest case for it would be the surest way to lock in the behaviour.
4. Changeset bump level. minor may or may not be right. The removed overloads (previously typed as GetTransactionApiResponseBase64<...> etc.) are gone from the public surface; anyone who typed a variable as Parameters<typeof decodeTransactionFromRpcResponse>[0] would see a different shape. And the JSON overload's transaction narrowing from ?: Transaction to ?: never will break code that read that field expecting Transaction | undefined. Both are arguably user-code bugs the new types now catch, but they're observable API changes. Given the package is at 7.0.0 and part of the fixed release group, a maintainer sanity-check on minor vs. major seems worthwhile — happy to defer to whatever the project's precedent is.
Notes for subsequent reviewers
- Please double-check point (3) — I reasoned through it from the type sources but didn't actually run
tscagainst a realgetBlockversioned-JSON call. If you have the repo checked out, adding one line to the typetest for that shape will confirm it in seconds. - Worth verifying the
@solana/rpc-apidemotion in point (2) against the fullpackages/transaction-introspection/src/tree, not just this file, before acting on it. - Everything else is straightforward and reads well. The new comment on
getLoadedAddressescorrectly captures why the runtime extraction still needs to be defensive even after the type generalisation.
|
Documentation Preview: https://kit-docs-jhzi5hjze-anza-tech.vercel.app |
9d7abec to
c31b449
Compare
Generalise the shape so that the transactions returned by `getBlock` and `getTransactionsForAddress` also satisfy it and can be decoded/parsed.
c31b449 to
bec07ce
Compare
Agreed, added
Agreed, removed the last import of
Added a typetest, and made
I think this is fine. The change widens the type, and the flagged breakages are user-code bugs as mentioned. |
| "@solana/errors": "workspace:*", | ||
| "@solana/instructions": "workspace:*", | ||
| "@solana/rpc-api": "workspace:*", | ||
| "@solana/rpc-types": "workspace:*", |
|
🔎💬 Inkeep AI search and chat service is syncing content for source 'Solana Kit Docs' |
|
Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up. |
3 participants
Problem
The new
transaction-introspectionAPI is currently tied to thegetTransactionRPC response, but other RPC methods also return the same shape transactions.Summary of Changes
Generalise the shape so that the transactions returned by
getBlockand the newgetTransactionsForAddressalso satisfy it and can be decoded/parsed.Fixes #1785