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

Starknet WebSocket specification proposal #211

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

amanusk
Copy link
Collaborator

@amanusk amanusk commented Jun 30, 2024

How to read this spec

  • Each subscription type is described by its own method, with relevant parameters.
  • The result of the subscription is a subscription ID, used to determine which stream a notification was received from, as well as an option to unsubscribe.
  • For each subscription type, there is a notification method describing the results received in each streamed event
  • A Reorg error is possible on all subscriptions, notifying that a reorg has occurred, and it is up to the client to treat this case according to their business logic

This change is Reviewable

@amanusk amanusk requested review from ArielElp and ittaysw June 30, 2024 08:00
api/starknet_ws_api.json Outdated Show resolved Hide resolved
"name": "subscription ID",
"description": "An identifier for this subscription stream used to associate events with this subscription.",
"schema": {
"type": "integer"
Copy link

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"
Copy link

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?)

Copy link
Collaborator Author

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

Copy link

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 Show resolved Hide resolved
"FELT": {
"$ref": "./api/starknet_api_openrpc.json#/components/schemas/FELT"
},
"BLOCK_SYNCING_STATUS": {
Copy link

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

Copy link
Collaborator Author

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": {
Copy link

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?

Choose a reason for hiding this comment

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

As I wrote above, I think this should be removed.

},
"reason": {
"title": "Closing reason",
"description": "The reason why the subscription was closed",
Copy link

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kkovaacs I took this error from the Pathfinder WS spec :),
Can you please elaborate on how the "reason" field is used?
Or is this some standard?
Thanks

Choose a reason for hiding this comment

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

No, this is an oversight / error in the specification. This is not really an error but a notification (or at least it should be). I think we should just remove this altogether.

"type": "object",
"description": "Data about reorganized blocks",
"properties": {
"starting_block_hash": {
Copy link

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

Copy link

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.

Copy link
Collaborator Author

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",
Copy link

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?

Choose a reason for hiding this comment

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

It's probably an internal error in the dapp I think. The only case Pathfinder returns false here is when you're specifying a non-existent subscription id. We could also just convert this into an error instead.

api/starknet_ws_api.json Show resolved Hide resolved
@amanusk amanusk marked this pull request as draft July 1, 2024 15:13
@ittaysw
Copy link

ittaysw commented Jul 2, 2024

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",
Copy link

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

Copy link
Collaborator Author

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",
Copy link

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

Copy link
Collaborator Author

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

@amanusk amanusk changed the title Starknet WS specification proposal Starknet WebSocket specification proposal Jul 2, 2024
Copy link
Collaborator

@ArielElp ArielElp left a 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?

}
},
"STARKNET_REORG_DETECTED": {
"code": 202,
Copy link

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",
Copy link

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a great point, can we express this in spec somehow? Or just in the description?

},
"methods": [
{
"name": "starknet_subscribeNewHeads",
Copy link

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": [],
Copy link

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"]
Copy link

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?

api/starknet_ws_api.json Show resolved Hide resolved
api/starknet_ws_api.json Outdated Show resolved Hide resolved
@ArielElp ArielElp changed the base branch from master to v0.8.0 July 25, 2024 10:36
@amanusk
Copy link
Collaborator Author

amanusk commented Aug 12, 2024

Latest changes based on discussions above

  • Add sender_address filter to pending transactions subscriptions
  • Add subscriptionReorg that can be received by Headers, Events, Transaction Status
  • Remove sync subscription and events
  • Nodes only allow WS sucscriptions after they are synced
  • Updated keys as in event filter

Open question:

  • Allowing to start a subscription from a previous block and "catch up" to the latest (applies to headers, events, transaction Status)
  • Returning subscription notifications one by one or as an array (applies to Headers, Events, Pending Transactions)

These are also questions for Full nodes and how much does this complicate implementation

api/starknet_ws_api.json Outdated Show resolved Hide resolved
],
"result": {
"name": "Unsubscription result",
"description": "True if the unsubscription was successful",

Choose a reason for hiding this comment

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

It's probably an internal error in the dapp I think. The only case Pathfinder returns false here is when you're specifying a non-existent subscription id. We could also just convert this into an error instead.

}
},
"errors": {
"WEBSOCKET_SUBSCRIPTION_CLOSED": {

Choose a reason for hiding this comment

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

As I wrote above, I think this should be removed.

},
"reason": {
"title": "Closing reason",
"description": "The reason why the subscription was closed",

Choose a reason for hiding this comment

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

No, this is an oversight / error in the specification. This is not really an error but a notification (or at least it should be). I think we should just remove this altogether.

@amanusk
Copy link
Collaborator Author

amanusk commented Aug 25, 2024

Update:
Added the option to subscribe to up to 1024 blocks back, and the accompanying error

@amanusk amanusk marked this pull request as ready for review August 25, 2024 15:02
api/starknet_ws_api.json Outdated Show resolved Hide resolved
},
{
"name": "block",
"summary": "The block to get notifications from, default is latest, limited to 1024 blocks back",
Copy link

Choose a reason for hiding this comment

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

Why is there a limit in the spec? We can let nodes determine their own limits if they need to, though from pathfinder's point of view it's probably fine to do this without a limit - this can simplify some of the client code.

Copy link

Choose a reason for hiding this comment

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

I see there are similar limits in other endpoints - maybe we should leave these up to nodes, if they want to limit users at all?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tend to agree that we can leave it up to nodes, I guess the original motivation was to avoid confusion on different rpc providers supporting different things. For now I'm leaving this in so that it can be part of 0.8.0-rc0, we can continue discussing this towards the final release.

api/starknet_ws_api.json Outdated Show resolved Hide resolved
Copy link
Collaborator

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 6 files at r1, 1 of 2 files at r2, 3 of 4 files at r4, 1 of 2 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 4 of 4 files at r8, all commit messages.
Reviewable status: all files reviewed, 28 unresolved discussions (waiting on @amanusk, @ittaysw, @kirugan, @kkovaacs, and @sistemd)

Copy link
Collaborator

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 24 unresolved discussions (waiting on @amanusk, @ittaysw, @kirugan, @kkovaacs, and @sistemd)

Copy link
Collaborator

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @amanusk, @ittaysw, @kirugan, @kkovaacs, and @sistemd)

Copy link
Collaborator

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

Dismissed @sistemd, and @sistemd from 23 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @amanusk)

@ArielElp ArielElp merged commit 497a7be into starkware-libs:v0.8.0 Sep 19, 2024
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.

6 participants