fix: remove block_id from starknet_subscribeTransactionStatus #2501
Conversation
vbar
force-pushed
the
vbar/remove-block-id
branch
from
4d60637 to
6138c2e
Compare
doc/rpc/v08/starknet_ws_api.json
Outdated
| }, | ||
| { | ||
| "name": "block_id", | ||
| "summary": "The block to get notifications from, default is latest, limited to 1024 blocks back", | ||
| "required": false, | ||
| "schema": { | ||
| "$ref": "./api/starknet_api_openrpc.json#/components/schemas/BLOCK_ID" | ||
| } |
There was a problem hiding this comment.
Same here -- I've already updated the specs in PR #2500, could be moved to a separate PR.
There was a problem hiding this comment.
Funnily enough, this must be updated, to make the rpc_routing test pass...
There was a problem hiding this comment.
Okay. The spec change is already on main though, so if you rebase this should be fixed.
There was a problem hiding this comment.
Hmm, I think something might have gone wrong with that rebase, because we're now re-adding block_id to the spec.
There was a problem hiding this comment.
I'm afraid git is too complex for me... Fixed in d161be1 .
| util::task::spawn_blocking(move |_| -> Result<_, RpcError> { | ||
| let mut conn = storage.connection().map_err(RpcError::InternalError)?; | ||
| let db = conn.transaction().map_err(RpcError::InternalError)?; | ||
| let first_block = db |
There was a problem hiding this comment.
Not sure I understand this part. first_block used to be the block_id parameter received in the request, while now it's essentially the latest block number.
Why do we need this at all?
I think the logic below could be simplified to use l1_block_number and tx_with_receipt only:
- if we have
tx_with_receipt(that is, the tx was already found in a block):- send RECEIVED
- send L2_ACCEPTED
- depending on the relation of the block in which the tx was found and
l1_block_number, send L1_ACCEPTED
- if we don't have the tx yet, just continue
There was a problem hiding this comment.
Well, not really understanding the logic, I went for a minimal change - but yes, it should be simplified...
There was a problem hiding this comment.
Well, not really understanding the logic, I went for a minimal change - but yes, it should be simplified...
...except sending RECEIVED unconditionally changes existing behavior: as far as I understood the discussion, the change should make the call behave as if the removed block_id always referred to the latest block... I tried to test that (in cb01b5c ), and the original implementation stopped sending the RECEIVED messages. I suppose a case could be made that "an event with the current known transaction status" is singular, and the updates before that are historical & not included in the new subscription. On the other hand, keeping the tested behavior unchanged also has its attractions... So, is the unconditional RECEIVED correct?
There was a problem hiding this comment.
@sistemd I think RECEIVED was just being sent so that we have the same set of statuses emitted irrespective of where those transaction statuses actually came from, right?
It might be worth double-checking the expected behavior with regard to these "historical" updates with the author(s) of the specification.
|
Another question: once we reach L1_ACCEPTED, which is effectively a final state, do we close the subscription? |
Well, |
Not quite. That test does test that there are no more messages sent -- but the subscription is still kept open, even though there's no possibility of further messages once we've reached a final status. In this state we're effectively waiting for the client to close the subscription -- maybe we should close it ourselves? @sistemd What do you think? |
@kkovaacs AFAIR, I decided to not close the subscription because there can be more messages coming in due to reorgs. The user can close the connection themselves if they explicitly don't care about reorgs. |
Hmm, that's a good point, I was missing reorgs... Makes sense! |
vbar
force-pushed
the
vbar/remove-block-id
branch
from
0aa4c1c to
1eb1176
Compare
vbar
force-pushed
the
vbar/remove-block-id
branch
from
cbd73ae to
a977074
Compare
Fixes #2495 .