feat: throw helpful error on duplicate transactions in composer - #591
feat: throw helpful error on duplicate transactions in composer#591mitre88 wants to merge 2 commits into
Conversation
|
Hi @joe-p @mrcointreau, this PR is ready for review. It fixes #366 by detecting duplicate transaction IDs in the composer group and throwing an actionable error that explains the suggestedParams cache cause and the fixes (note, lease, validityWindow, rounds). Unit tests cover the failure path and the differentiated-note success path. Validation: vitest, tsc, eslint all pass. |
9b0847f to
5f29ea2
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds early, descriptive detection of duplicate transactions within a composed transaction group, addressing cases where cached suggested params can lead to identical txIDs and an unhelpful network rejection.
Changes:
- Add duplicate-transaction detection in
build()andbuildTransactions()and throw a guidance-rich error. - Add unit tests covering both the failure case (identical txns) and a success case (differentiated notes).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/types/composer.ts | Adds duplicate txID detection and throws a descriptive error during build paths. |
| src/types/composer.spec.ts | Adds unit tests to validate duplicate detection and a note-based differentiation scenario. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for (let index = 0; index < transactions.length; index++) { | ||
| const txId = transactions[index].txID() | ||
| const firstIndex = firstIndexById.get(txId) | ||
| if (firstIndex !== undefined) { |
| throw new Error( | ||
| `Transaction group contains duplicate transactions (same transaction ID) at indexes ${firstIndex} and ${index} (txID: ${txId}). ` + | ||
| `This often happens when suggestedParams are cached, so identical calls get the same firstValid/lastValid rounds. ` + | ||
| `Make each transaction unique (e.g. different note, lease, validityWindow, firstValidRound/lastValidRound, or other fields), ` + | ||
| `or refresh suggested params (setSuggestedParamsCacheTimeout(0) or wait for the cache to expire and call getSuggestedParams again).`, | ||
| ) |
|
This PR should be targeting |
5f29ea2 to
6f931b4
Compare
Update (author)Rebased onto latest
GitHub Actions on this fork PR is currently Happy to adjust if review wants a different audit policy than the exclusions already used on sibling PRs. |
Detect identical transaction IDs within a composed group and fail early with guidance about cached suggestedParams and how to differentiate transactions (note, lease, validity window). Closes algorandfoundation#366.
6f931b4 to
7062441
Compare
|
Rebased onto Duplicate-detection tests pass locally (
|
Call out 0-based indexes and keep the suggested-params hint on AlgorandClient APIs so TransactionComposer users are not pointed at methods that do not exist on the composer.
|
Thanks for the base-branch note. This PR now targets The node-ci |
Summary
build()andbuildTransactions().suggestedParamscache cause and how to make each txn unique (note,lease,validityWindow, rounds, or refresh params).Context
Fixes #366. Duplicate transactions are easy to create accidentally when
suggestedParamsare cached (same first/last valid), e.g. multiple identical gas/app calls in one group. Today the network rejects the group with a less actionable error; this fails early with guidance.Validation
npx vitest run src/types/composer.spec.ts -t "duplicate" npx tsc --noEmit npx eslint src/types/composer.ts src/types/composer.spec.ts