Migrated transaction.js file to TS#924
Conversation
| // TODO: types/index.d.ts declares this as `number`, but the XDR type is | ||
| // Duration = Uint64 (64-bit), which can exceed Number.MAX_SAFE_INTEGER. | ||
| // This is a breaking type change that should be surfaced when types/index.d.ts is updated. | ||
| _minAccountSequenceAge?: xdr.Uint64; |
There was a problem hiding this comment.
This is sort of a breaking change, so we might want to document it.
|
Size Change: +2.52 kB (+0.07%) Total Size: 3.61 MB
|
| export class TransactionBase< | ||
| TTx extends xdr.FeeBumpTransaction | xdr.Transaction | xdr.TransactionV0 | ||
| > { | ||
| _tx: TTx; |
There was a problem hiding this comment.
Needed to make this update to get the types properly working in Transaction (and FeeBumpTransaction later).
| */ | ||
| export function isValidDate(d) { | ||
| // isnan is okay here because it correctly checks for invalid date objects | ||
| // eslint-disable-next-line no-restricted-globals |
There was a problem hiding this comment.
ESLint was complaining about it.
There was a problem hiding this comment.
Pull request overview
Migrates the core Transaction implementation from JavaScript to TypeScript, aligning it with the repo’s ongoing TS migration and improving type-safety around transaction envelopes and preconditions.
Changes:
- Converted
src/transactionimplementation to TypeScript with explicit XDR/operation typings. - Generalized
TransactionBaseto be generic over transaction XDR types (TransactionV0/Transaction/FeeBumpTransaction). - Added generated type validation declarations for
Transaction/ updatedTransactionBasedeclarations.
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| type_validation/transaction_base.d.ts | Updates TransactionBase declarations to be generic over supported XDR transaction types. |
| type_validation/transaction.d.ts | Adds generated type declarations for the migrated Transaction class. |
| src/transaction_builder.js | Removes an ESLint disable comment in isValidDate. |
| src/transaction_base.ts | Makes TransactionBase generic over XDR transaction variants. |
| src/transaction.ts | New TS implementation of Transaction, including signature base, envelope conversion, and claimable balance ID logic. |
Comments suppressed due to low confidence (3)
src/transaction.ts:294
- In
signatureBase(), the AccountID discriminant is hard-coded asBuffer.alloc(4). This relies onPUBLIC_KEY_TYPE_ED25519always being0and encoded as 4 zero bytes; using the XDR enum to generate the discriminant (as was done previously) avoids baking in that assumption and is clearer about intent.
src/transaction.ts:393 HashIdPreimageOperationIdexpectsAccountId sourceAccountandSequenceNumber seqNumin XDR. Even though these are currently typedefs ofPublicKeyandInt64, constructing them viaxdr.AccountId/xdr.SequenceNumberwould better match the schema and existing code patterns (e.g., TransactionBuilder) and reduces the risk of subtle type/behavior drift if the typedefs change.
src/transaction.ts:200- The JSDoc for
minAccountSequenceAgestill states@type {number}, but the implementation stores/returns an XDRUint64(Duration) and the generated.d.tsreflects that. Please update the JSDoc (and any related docs) to match the actual return type to avoid misleading consumers.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Tests will be updated in a separate PR because they need
TransactionBuilder: #923