Skip to content
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

TransactionWorker to fire events a dapp can listen to #255

Merged
merged 3 commits into from
Jan 24, 2024

Conversation

0xmaayan
Copy link
Collaborator

@0xmaayan 0xmaayan commented Jan 17, 2024

Description

The transaction management is a send-and-forget process in a way that once transactions are sent to worker, the dapp is not aware of what is going on unless a potential error happens.

This PR adds logic for the TransactionWorker module to fire events during a worker task and dapp can listen to these events.

aptos.transaction.batch.on(TransactionWorkerEvents.ExecutionFinish, async (data) => {
   console.log("execution finish", data);
});

Note:
Currently, dapp uses aptos.batchTransactionsForSingleAccount() to start the worker. We now introduce a dedicated namespace under aptos.transaction.batch where one can use aptos.transaction.batch.forSingleAccount() same as before. This is for potential future enhancement of the worker and to conform with the sdk design.

Test Plan

pnpm test
batch_mint and batch_funds for examples

Related Links

/**
* Internal function to start listening to transaction worker events
*
* TODO - should we ask events to listen to as an input?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless it's really expense to emit events it might not be worth the complexity.

src/transactions/management/transactionWorker.ts Outdated Show resolved Hide resolved
src/transactions/management/transactionWorker.ts Outdated Show resolved Hide resolved
@@ -157,12 +165,21 @@ export class TransactionWorker {
// transaction sent to chain
this.sentTransactions.push([sentTransaction.value.hash, sequenceNumber, null]);
// check sent transaction execution
this.emit(
TransactionWorkerEvents.TransactionSent,
`transaction hash ${sentTransaction.value.hash} has been commited to chain`,
Copy link
Contributor

@banool banool Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the data being emitted? If so, shouldn't we emit something structured instead? That way the callback can actually do something with the data. So it emits some kind of TransactionSentEventData that has a txnHash field. The user can use that type in the callback too rather than data: any.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call! I have updated the worker/management to have a types returned data

Comment on lines +36 to +37
export interface TransactionWorkerEvents {
transactionSent: (data: SuccessEventData) => void;
transactionSendFailed: (data: FailureEventData) => void;
transactionExecuted: (data: SuccessEventData) => void;
transactionExecutionFailed: (data: FailureEventData) => void;
executionFinish: (data: ExecutionFinishEventData) => void;
}
Copy link
Contributor

@kaw2k kaw2k Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, just double checking I understand each event:

  • transactionSent is fired after a transaction gets sent to the chain
  • transactionSendFailed is fired if there is an error sending the transaction to the chain
  • transactionExecuted is fired when a single transaction has run successfully
  • transactionExecutionFailed is run if a single transaction fails in execution
  • executionFinish is run when the queue has been emptied

So if someone wanted to make an async function to batch requests, it would look like this:

// Pseudo code
type TxnHash = string;
function batchTransactionExample(sender: Account, payload: InputGenerateTransactionPayloadData[]): Promise<{
	succeeded_txns: TxnHash[],
	failed_txns: TxnHash[]
}> {
	return new Promise(async (resolve, reject) => {
		let succeeded_txns = [];
		let failed_txns = [];
		
		aptos.transaction.batch.forSingleAccount({sender, data: payload})

		// Handle transaction successes, we don't care about sent, just executed
		aptos.transaction.batch.on(TransactionWorkerEvents.TransactionExecuted, async (data: any) => {
			succeeded_txns.push(data.txn_hash);
		});

		// Handle transaction failures (execution or send)
		aptos.transaction.batch.on(TransactionWorkerEvents.TransactionExecutionFailed, async (data: any) => {
			failed_txns.push(data.txn_hash);
		});
		aptos.transaction.batch.on(TransactionWorkerEvents.TransactionSendFailed, async (data: any) => {
			failed_txns.push(data.txn_hash);
		});

		// Handle resolving the promise
		aptos.transaction.batch.on(TransactionWorkerEvents.ExecutionFinish, async (data: any) => {
			resolve({ succeeded_txns, failed_txns })
		});
	})
}

I'm guessing the caller will have to unsubscribe from the emitter? That is something we would want to highlight in the two examples potentially.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep! I'll add some doc comments - good catch!

);
});
// worker finished execution, we can now unsubscribe from event listeners
aptos.transaction.batch.removeAllListeners();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

Copy link
Contributor

@kaw2k kaw2k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting this in!!!

@0xmaayan 0xmaayan merged commit 8221132 into main Jan 24, 2024
7 checks passed
@0xmaayan 0xmaayan deleted the event_transaction_management branch January 24, 2024 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants