-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[TS SDK] Add account transaction management #8854
Conversation
a5f14ec
to
becfa82
Compare
ecosystem/typescript/sdk/examples/typescript/transactions_management.ts
Outdated
Show resolved
Hide resolved
ecosystem/typescript/sdk/src/transactions/tests/account_sequence_number.test.ts
Outdated
Show resolved
Hide resolved
ecosystem/typescript/sdk/src/transactions/tests/account_sequence_number.test.ts
Outdated
Show resolved
Hide resolved
ecosystem/typescript/sdk/src/transactions/transaction_worker.ts
Outdated
Show resolved
Hide resolved
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.
Looks good overall, some nits but good to land :)
303b340
to
4759193
Compare
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.
good start!
I wonder if it might be useful for me to host a session that goes over what I did in Python?
Some areas of focus in this PR:
- Needs error handling, if there's any error, the service cannot recover.
- Limited concurrency -- we should probably make more workers that just pull on the next available transaction and another worker that just processes transactions.
- Logging would benefit from a cleanup
- Timestamps to indicate decent throughput would be good too
- Descriptions of each major components
I'd also be curious to understand how you landed on EventEmitter3 versus say worker threads and making it a js only concept. It might be useful to come up with a story around this and evaluate if it even makes sense to have a high throughput construct for browsers or just node.
ecosystem/typescript/sdk/examples/typescript/transactions_management.ts
Outdated
Show resolved
Hide resolved
ecosystem/typescript/sdk/examples/typescript/transactions_management.ts
Outdated
Show resolved
Hide resolved
ecosystem/typescript/sdk/examples/typescript/transactions_management.ts
Outdated
Show resolved
Hide resolved
ecosystem/typescript/sdk/examples/typescript/transactions_management.ts
Outdated
Show resolved
Hide resolved
ecosystem/typescript/sdk/src/transactions/transaction_worker.ts
Outdated
Show resolved
Hide resolved
ecosystem/typescript/sdk/src/transactions/transaction_worker.ts
Outdated
Show resolved
Hide resolved
30507bb
to
acc32e8
Compare
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.
looking good
ecosystem/typescript/sdk/examples/typescript/transactions_management.ts
Outdated
Show resolved
Hide resolved
const res = await Promise.all(waitFor); | ||
console.log(`transactions verified`); | ||
console.log(new Date().toTimeString()); | ||
for (const account in res) { |
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.
line numbers don't make sense, could you please update or hit me up with the lines that do this?
the reason I made this comment was below.. leaving comment there
ecosystem/typescript/sdk/examples/typescript/transactions_management.ts
Outdated
Show resolved
Hide resolved
async synchronize(): Promise<void> { | ||
if (this.lastUncommintedNumber === this.currentNumber) return; | ||
|
||
while (this.lock) { |
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'm not seeing how this connects. Considering that this is a public function, it is a bit dangerous to not get the lock here. Especially with calls into initialize..
ecosystem/typescript/sdk/src/transactions/transaction_worker.ts
Outdated
Show resolved
Hide resolved
ecosystem/typescript/sdk/src/transactions/transaction_worker.ts
Outdated
Show resolved
Hide resolved
ef8f9e8
to
d8d1b36
Compare
ecosystem/typescript/sdk/src/transactions/transaction_worker.ts
Outdated
Show resolved
Hide resolved
37d2920
to
19d1543
Compare
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.
looking good
ecosystem/typescript/sdk/examples/typescript/transactions_management.ts
Outdated
Show resolved
Hide resolved
console.log("sentFailed", data); | ||
}); | ||
|
||
transactionWorker.on(TransactionWorkerEvents.TransactionExecuted, async (data) => { |
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.
so this is where you're confirming success and syncing?
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.
technically we confirm it here https://github.com/aptos-labs/aptos-core/pull/8854/files#diff-52d3c5103be2ebfaf76511751a50a8fc3ce5f720a77ddd6fed4e74891074edb0R172 , if execution succeed/failed we emit a TransactionExecuted/TransactionExecutionFailed event that the client can listen to and do whatever they want.
async synchronize(): Promise<void> { | ||
if (this.lastUncommintedNumber === this.currentNumber) return; | ||
|
||
while (this.lock) { |
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.
👋
fb45c61
to
e9083be
Compare
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.
unblocking, but please wait for the lock in synchronize
async synchronize(): Promise<void> { | ||
if (this.lastUncommintedNumber === this.currentNumber) return; | ||
|
||
this.lock = true; |
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.
you can't get the lock without waiting for it, you could theoretically have multiple calls to synchronize... I don't think anything would really bad happen because of the excessive serial nature here, but....
transaction management layer add sequnce number tests transaction worker class add transaction worker and queue class add start and stop to transaction worker add txn worker tests write full flow test add account transactions management
e9083be
to
2583fc8
Compare
2583fc8
to
ebf3233
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
* add account transaction management transaction management layer add sequnce number tests transaction worker class add transaction worker and queue class add start and stop to transaction worker add txn worker tests write full flow test add account transactions management * add method comments * address comments * implement async queue * events as enum * address feedback
* add account transaction management transaction management layer add sequnce number tests transaction worker class add transaction worker and queue class add start and stop to transaction worker add txn worker tests write full flow test add account transactions management * add method comments * address comments * implement async queue * events as enum * address feedback
Description
based on #7987
This PR provides a layer for managing and submitting as many transactions from a single account at once.
In addition, provide an example demonstrates the transaction management layer and how one can implement it.
Test Plan
tests are passing
output from running the example against devnet using a wifi connection with download speed of 23.38 mbps and upload speed of 8.40 mbps