-
Notifications
You must be signed in to change notification settings - Fork 11
Composer integration #457
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
base: decoupling
Are you sure you want to change the base?
Composer integration #457
Conversation
| return result | ||
| })() | ||
| : undefined, | ||
| rejectVersion: bigIntCodec.decodeOptional(transactionDto.aprv), |
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.
| rejectVersion: bigIntCodec.decodeOptional(transactionDto.aprv), | |
| rejectVersion: numberCodec.decodeOptional(transactionDto.aprv), |
| /** | ||
| * The lowest application version for which this transaction should immediately fail. 0 indicates that no version check should be performed. | ||
| */ | ||
| rejectVersion?: bigint |
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.
| rejectVersion?: bigint | |
| rejectVersion?: number |
| apep?: number | ||
|
|
||
| /** Reject version */ | ||
| aprv?: bigint | number |
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.
| aprv?: bigint | number | |
| aprv?: number |
| // Populate the composer with method calls | ||
| if ('transactions' in transaction) { | ||
| transaction.methodCalls.forEach((m, i) => sendParams.atc!['methodCalls'].set(i + baseIndex, m)) | ||
| transaction.methodCalls.forEach((m, i) => sendParams.transactionComposer!['methodCalls'].set(i + baseIndex, m)) |
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.
This feels hacky. Maybe it's ok because it's in the legacy code.
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.
yeh it's pretty hacky. The problem comes from here
const transaction = await txn(transactionCreator)(params)
this causes the trasaction to be a txnWithSigner => when it's added to the composer, the methodCalls map isn't updated.
| // The state of the atc is unknown, whether it has resource populated or not | ||
| // In this version, we call composer.simulate which doesn't do resource population | ||
|
|
||
| const simulateOptions = { |
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.
These options appear to be different from the original.
| * Builds an ABI method call transaction and any other associated transactions represented in the ABI args. | ||
| * @param includeSigner Whether to include the actual signer for the transactions. | ||
| * If you are just building transactions without signers yet then set this to `false`. | ||
| * Compose all of the transactions without signers and return the transaction objects directly along with any ABI method calls. |
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.
This does return signers as well
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.
The jsdocs confused me as first too because this method returns the signer Map. I think it means that the transactions returned doesn't have signers attached. Should I change the docs?
src/types/composer.ts
Outdated
|
|
||
| const builtTransactions = await this.buildTransactions() | ||
| const transactions = |
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.
Doesn't buildTransactions take care of grouping?
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.
No, buildTransactions doesn't do grouping. Grouping is applied in build, after resource population and fee coverage.
You mentioned this makes me think that the names buildTransactions and build are confusing. I kept them because:
buildTransactionsexisted in the composer. It does similar functionality.buildexisted in the composer, but private. Inmain, resource population + fee coverage happen insend, for this version, I lifted them tobuild
I'm happy to find better names for them but that means breaking changes.
| const error = new Error(errorMessage) | ||
|
|
||
| if (Config.debug) { | ||
| await Config.events.emitAsync(EventType.TxnGroupSimulated, { simulateResponse }) | ||
| await Config.events.emitAsync(EventType.TxnGroupSimulated, { simulateTransaction: simulateResponse }) |
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.
The utils-ts-debug repo provides a handler for this event, so we need to think about migration and breaking changes here.
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.
I'll add a note to the migration doc
| }) | ||
|
|
||
| // Sign transactions in parallel for each signer | ||
| const signerEntries = Array.from(signerGroups) |
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.
Can you double check this logic against algosdk to ensure it has the same behaviour?
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.
I assume you mean the logic to sign transactions in parallel. The sdk does the same
const batchedSigs = await Promise.all(
orderedSigners.map(([signer, indexes]) => signer(txnGroup, indexes))
);
src/types/composer.ts
Outdated
| return ( | ||
| ctxn.type === 'appCall' || | ||
| ctxn.type === 'methodCall' || | ||
| (ctxn.type === 'txnWithSigner' && ctxn.data.txn.type === TransactionType.AppCall) |
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.
What about asyncTxn?
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.
I'll remove this function, it isn't needed anymore. In build (where it was used) I have access to the built transaction, I can check for the transaction type directly.
db98a4b to
09000c2
Compare
| @@ -547,19 +234,19 @@ export interface BuiltTransactions { | |||
| signers: Map<number, algosdk.TransactionSigner> | |||
| } | |||
|
|
|||
| class BuildComposerTransactionsError extends Error { | |||
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.
@neilcampbell I created this error type for errors during build transactions. I think this is helpful because before this, in debug mode, if something goes wrong during resource population, we will need to run simulate again in the error handler. Using this error type, we can surface to simulation result to the error handler => eliminate the need to simulate again.
| expect(clonedAppClient.appId).toBe(appClient.appId) | ||
| expect(clonedAppClient.appName).toBe(appClient.appName) | ||
| expect((await clonedAppClient.createTransaction.bare.call()).sender).toBe(testAccount2.addr.toString()) | ||
| expect((await clonedAppClient.createTransaction.call({ method: 'default_value', args: ['test value'] })).transactions[0].sender).toBe( |
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.
I had to update this test to use valid transaction params because the transaction creator now calls composer.build(), resulted in a simulate call.
| const failedGroup = simulateResponse?.txnGroups[0] | ||
| if (failedGroup?.failureMessage) { | ||
| const errorMessage = `Transaction failed at transaction(s) ${failedGroup.failedAt?.join(', ') || 'unknown'} in the group. ${failedGroup.failureMessage}` | ||
| const simulateResponse = await this.algod.simulateTransaction(simulateRequest) |
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.
@neilcampbell The name of the return type of simulateTransaction is SimulateTransaction while algosdk uses SimulateResponse. I think we should update our algod to use SimulateResponse too.
Proposed Changes