-
Notifications
You must be signed in to change notification settings - Fork 70
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
Starknet WebSocket specification proposal #211
base: master
Are you sure you want to change the base?
Conversation
"name": "subscription ID", | ||
"description": "An identifier for this subscription stream used to associate events with this subscription.", | ||
"schema": { | ||
"type": "integer" |
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.
if integer, it can't be "0x..." form.
"$ref": "#/components/errors/WEBSOCKET_SUBSCRIPTION_CLOSED" | ||
}, | ||
{ | ||
"$ref": "#/components/errors/STARKNET_REORG_DETECTED" |
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 an error is something that the callee responds with. In this case the callee is the dapp. How can it detect a reorg and why should it report it to the node (what will the node do with it?)
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.
Indeed the intention was for the node to send the error, not the Dapp. The Dapp needs to act upon this.
The error could be received async, and not in direct response to a request
Open for suggestions for how to best describe this 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 think another type of subscription.
api/starknet_ws_api.json
Outdated
"FELT": { | ||
"$ref": "./api/starknet_api_openrpc.json#/components/schemas/FELT" | ||
}, | ||
"BLOCK_SYNCING_STATUS": { |
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 is not compatible in properties name with SYNC_STATUS from the "pull" RPC
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 are right, didn't realize it was already defined. Fixing
} | ||
}, | ||
"errors": { | ||
"WEBSOCKET_SUBSCRIPTION_CLOSED": { |
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.
Seems like this is returned by the dapp when an event is sent. if so, the node knows the subscription (it is sending it), so why report it back?
}, | ||
"reason": { | ||
"title": "Closing reason", | ||
"description": "The reason why the subscription was closed", |
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 will the node do with this?
"type": "object", | ||
"description": "Data about reorganized blocks", | ||
"properties": { | ||
"starting_block_hash": { |
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 is returned by a dapp in the above. It wouldn't know all these details.
Maybe you meant this to be some metadata that a node can send to a dapp? If so it's another subscription type
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.
and seems like this is very common with SYNC_STATUS, so maybe reuse.
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.
Indeed very similar, but there is no use for current_block
@ArielElp WDYT, makes sense to split SYNC_STAUTS, or duplicate?
], | ||
"result": { | ||
"name": "Unsubscription result", | ||
"description": "True if the unsubscription was successful", |
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 should the dapp do if it is not successful?
What is the behavior of these subscriptions when a node is syncing? E.g. I subscribed to events, will i get all events as it is going through blocks? A node may be syncing back as well, what is the meaning of 'latest' then? And if two nodes are syncing on the same range, one forward, one backward, will they send different events on the stream? Seems to me like we should just not allow subscribing when a node is syncing, only when it is fully synced. Then we can also remove the syncing status call. |
] | ||
}, | ||
{ | ||
"name": "starknet_subscribePendingTransactions", |
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.
maybe instead have a 'block' parameter to starknet_subscribeTransactions which will also be more uniform with 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.
This should be an interim solution until PendingTransactions
reflects transactions in the mempool.
I don't think it makes sense to stream all transactions if they are part of an already complete block
] | ||
}, | ||
{ | ||
"name": "starknet_subscribeSyncing", |
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 is the use case here? Do we expect nodes to start syncing, then stop, then start again?
This can be also the stream for reorg notification
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.
Worth a discussion in the nodes-collab meeting. It's currently here since Ethereum has such endpoint.
The usecase I can think of is indeed to only start some process after sync is complete
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.
Reviewable status: 0 of 7 files reviewed, 16 unresolved discussions (waiting on @amanusk and @ittaysw)
api/starknet_ws_api.json
line 325 at r1 (raw file):
Previously, ittaysw (Ittay Dror) wrote…
Yes
Why not have the response of tx status subscription be allOf tx_hash and TXN_STAUTS? NEW_TXN_STATUS is weird.
api/starknet_ws_api.json
line 380 at r1 (raw file):
Previously, amanusk (amanusk) wrote…
Indeed very similar, but there is no use for
current_block
@ArielElp WDYT, makes sense to split SYNC_STAUTS, or duplicate?
I think we can keep as-is, but up to you
api/starknet_ws_api.json
line 59 at r3 (raw file):
"required": false, "schema": { "$ref": "#/components/schemas/FELT"
ADDRESS
api/starknet_ws_api.json
line 63 at r3 (raw file):
}, { "name": "keys",
keys is a nested array, e.g. [[],[1, 2], [3]], means (k1=1 or k1=2) and k3=3, with no constraints on k0
api/starknet_ws_api.json
line 105 at r3 (raw file):
"name": "result", "schema": { "$ref": "./api/starknet_api_openrpc.json#/components/schemas/EMITTED_EVENT"
am I necessarily getting events one at a time? does an array of EMITTED_EVENT make sense here?
api/starknet_ws_api.json
line 200 at r3 (raw file):
"name": "result", "description": "Either a tranasaction hash or full transaction details, based on subscription", "schema": {
same as my events question, are we insisting on one item or can we can a chunk of results?
{ | ||
"name": "result", | ||
"schema": { | ||
"$ref": "./api/starknet_api_openrpc.json#/components/schemas/BLOCK_HEADER" |
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.
why this is part of params
field?
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 is how Open RPC spec describes notifications, which is the object we want to describe.
This is also the case for how Juno works right now ("result" within "params")
https://juno.nethermind.io/websocket#subscribe-to-newly-created-blocks
} | ||
}, | ||
"STARKNET_REORG_DETECTED": { | ||
"code": 202, |
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.
if we following json-rpc spec (which might be the case because WEBSOCKET_SUBSCRIPTION_CLOSED
code is in valid range) then we should follow it here as well:
-32000 to -32099 Server error Reserved for implementation-defined server-errors.
] | ||
}, | ||
{ | ||
"name": "starknet_subscribeTransactionStatus", |
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.
do we want a TTL here? E.g. maybe the tx is not known (was dropped from the mempool, deemed invalid, in some old block, typo in hash), and the subscription will stay forever?
}, | ||
"methods": [ | ||
{ | ||
"name": "starknet_subscribeNewHeads", |
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.
For all subscriptions: Say the server wants to drop a subscription (to conserve space, because maybe it suspects they are not listened to), without closing the websocket, should there be a message to state this, so the client can re-register?
"name": "starknet_subscribeNewHeads", | ||
"summary": "New block headers subscription", | ||
"description": "Creates a WebSocket stream which will fire events for new block headers", | ||
"params": [], |
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.
perhaps allow a block number? E.g. say a client got disconnected and now connects the websocket again and wants to receive all interim blocks? Maybe an 'n' for 'n latest'?
"required": false, | ||
"schema": { | ||
"type": "string", | ||
"enum": ["pending", "latest"] |
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.
similar to new heads, maybe there should be an 'n' here to close gaps?
"name": "starknet_subscribePendingTransactions", | ||
"summary": "New Pending Transactions subscription", | ||
"description": "Creates a WebSocket stream which will fire events when a new pending transaction is added. While there is no mempool, this notifies of transactions in the pending block", | ||
"params": [ |
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.
maybe 'sender_address' filter? otherwise, what is the use case? get all transactions added to a block as they are added?
"description": "Notification to the client of a new block header", | ||
"params": [ | ||
{ | ||
"name": "subscription_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.
Can we make these lowercase? I think in other parts of the JSON-RPC specification we're using _id
and not uppercase.
How to read this spec
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)