-
Notifications
You must be signed in to change notification settings - Fork 87
Add subscriptions for pre-confirmed transactions #323
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
Add subscriptions for pre-confirmed transactions #323
Conversation
* Add endpoint to subscript for pre-confirmed transacitons * Add endpoint to subscribe for pre-confirmed events * Revise description of events subscription to explain the latest block it notifies about is the latest block * Update the block_id of subscription to be a subset of block_id
Co-authored-by: Thiago Ribeiro <[email protected]>
Co-authored-by: Thiago Ribeiro <[email protected]>
| "$ref": "#/components/schemas/SUBSCRIPTION_BLOCK_ID" | ||
| } | ||
| }, | ||
| { |
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.
When using starknet_subscribeEvents with block_id: Latest and finality_status: PRE_CONFIRMED, we're supposed to return all events in the latest block before starting to return events in the pre-confirmed block, correct? ie. there's no way to tell the node to start from the pre-confirmed block
(not saying it's a bad or good thing, I'm just making sure I'm reading this right)
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.
Yes, as is the case in 0.8 with events of Pending transactions
| "summary": "A vector of finality statuses to receive updates for, default is [ACCEPTED_ON_L2]", | ||
| "required": false, | ||
| "schema": { | ||
| "type": "array", |
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 think I'm missing why this field is an array instead of a string like starknet_subscribeEvents
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 idea was to allow to get only a subset of transaction, based on the finality you are looking fore. Only accepted_on_l2 if you don't want duplicates, and add pre_confirmed if more recent transactions are needed, and duplicates are acceptable
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.
BTW, do we really want to support the ACCEPTED_ON_L1 finality status here?
It'd be great if this subscribeNewTransactionReceipts and subscribeNewTransactions subscriptions would have the same semantics (and filtering capabilities).
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.
@kkovaacs, I agree. Supporting ACCEPTED_ON_L1 is not a good idea here; it can create a large batch of receipts received at once.
Re having the same semantics, we can have them as close as we can, but there are no receipts for CANDIDATE and RECEIVED so these would also be excluded from the options for receipts
api/starknet_ws_api.json
Outdated
| { | ||
| "not": { | ||
| "type": "string", | ||
| "enum": ["ACCEPTED_ON_L1", "CANDIDATE", "RECEIVED"] |
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.
Here we are creating more exceptions than acceptance (3 out of 5 tags from the TXN_STATUS type are being excluded).
Why not use the TXN_FINALITY_STATUS instead, and only exclude the ACCEPTED_ON_L1 tag?
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.
Yes, good point.
I was trying to go for "this is the same as NewTransactions", but this is just not the case as receipts are not generated for CANDIDATE and RECEIVED transactions.
Fixed to your suggestion
Co-authored-by: KOVACS Krisztian <[email protected]>
35651cb
into
starkware-libs:release/v0.9.0-rc.3
Changes
Checklist:
npm run validate_allnpm run format