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_subscribeTransactionStatus websocket method #2210

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pnowosie
Copy link
Contributor

@pnowosie pnowosie commented Oct 11, 2024

Fixes #2209

Comments:

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 73.90%. Comparing base (eb1f2ca) to head (2d073dd).

Files with missing lines Patch % Lines
rpc/subscriptions.go 76.92% 13 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2210      +/-   ##
==========================================
- Coverage   74.33%   73.90%   -0.43%     
==========================================
  Files         110      110              
  Lines       11531    11614      +83     
==========================================
+ Hits         8571     8583      +12     
- Misses       2290     2363      +73     
+ Partials      670      668       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pnowosie pnowosie force-pushed the pnowosie/2209-subscribe-tx-status-ws branch 2 times, most recently from c5eb695 to abfec15 Compare October 22, 2024 12:07
@pnowosie pnowosie marked this pull request as ready for review October 23, 2024 11:02
@pnowosie pnowosie force-pushed the pnowosie/2209-subscribe-tx-status-ws branch from dae4ff2 to bffd2bf Compare October 23, 2024 12:13
rpc/handlers.go Outdated Show resolved Hide resolved
rpc/events.go Outdated
return err
}

func (h *Handler) SubscribeTxnStatus(ctx context.Context, txHash felt.Felt, _ *BlockID) (*SubscriptionID, *jsonrpc.Error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As per the specs, we also need to support historical blocks. Or is it not possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not possible. I added a comment explaining this.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should return error in this case? otherwise users will be confused why it's behaving differently from other possible impl.

@pnowosie pnowosie force-pushed the pnowosie/2209-subscribe-tx-status-ws branch from 0ce1ae2 to b170da1 Compare October 29, 2024 12:12
@pnowosie pnowosie force-pushed the pnowosie/2209-subscribe-tx-status-ws branch from b170da1 to b617595 Compare November 13, 2024 12:59
@pnowosie pnowosie force-pushed the pnowosie/2209-subscribe-tx-status-ws branch 4 times, most recently from 6b842a7 to e65a8a4 Compare December 13, 2024 13:53
jsonrpc/server.go Outdated Show resolved Hide resolved
rpc/events.go Outdated
@@ -9,11 +9,20 @@ import (
"github.com/NethermindEth/juno/jsonrpc"
)

const (
MaxBlocksBack = 1024
Copy link
Contributor

Choose a reason for hiding this comment

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

There is new constant introduced in Aneeque's PR maxBlocksBack in rpc/handlers.go. If both of them related consider to reuse existing one

rpc/events.go Outdated
}

id := h.idgen()
subscriptionCtx, subscriptionCtxCancel := context.WithCancel(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need to call defer subscriptionCtxCancel() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's called by sub.unsubscribe()

rpc/events.go Outdated
}()

if err := h.sendTxnStatus(sub.conn, wrapResult(lastKnownStatus), id); err != nil {
h.log.Warnw("Error while sending Txn status", "txHash", txHash)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe correct severity would be Error instead of Warnw ? Also please add "err", err as last arguments

rpc/events.go Outdated
case <-headerSub.Recv():
lastKnownStatus, rpcErr = h.TransactionStatus(subscriptionCtx, txHash)
if rpcErr != nil {
h.log.Warnw("Failed to get Tx status", "txHash", txHash, "rpcErr", rpcErr)
Copy link
Contributor

Choose a reason for hiding this comment

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

same question: maybe it should be an error ?

rpc/events.go Outdated
}

// Stop when final status reached and notified
if isFinal(lastSendStatus) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is it according to the spec ?

Copy link
Contributor Author

@pnowosie pnowosie Dec 16, 2024

Choose a reason for hiding this comment

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

It's not in written the spec.
I remember we are talking about the final statuses (=no next status in diagram)
I cannot recall why - if it was me who deducted that the subscription should be dropped in this case or it was in the group conversation 🤔

utils/check.go Outdated Show resolved Hide resolved
rpc/handlers.go Outdated
@@ -512,7 +523,7 @@ func (h *Handler) MethodsV0_7() ([]jsonrpc.Method, string) { //nolint: funlen
Handler: h.SpecVersionV0_7,
},
{
Name: "juno_subscribeNewHeads",
Name: "starknet_subscribeNewHeads",
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need to update starknet_subscribeNewHeads in this PR ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll revert 👍 - I based on Han's PR - and then reverted when Anique's PR get merged

@pnowosie pnowosie force-pushed the pnowosie/2209-subscribe-tx-status-ws branch 2 times, most recently from bd4e154 to a552376 Compare December 16, 2024 12:21
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please move SubscribeTxnStatus to subscription.go?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please move the following test to subscription_test.go

rpc/events.go Outdated

// sendHeader creates a request and sends it to the client
func (h *Handler) sendTxnStatus(w jsonrpc.Conn, status *NewTransactionStatus, id uint64) error {
resp, err := json.Marshal(jsonrpc.Request{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please. use SubscriptionResponse struct instead of jsonrpc.Request

rpc/events.go Outdated
h.subscriptions[id] = sub
h.mu.Unlock()

statusSub := h.txnStatus.Subscribe()
Copy link
Contributor

Choose a reason for hiding this comment

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

No one is sending on this subscription nor is anyone reading from it so why is this required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch 👍

rpc/events.go Outdated
select {
case <-subscriptionCtx.Done():
return
case <-headerSub.Recv():
Copy link
Contributor

Choose a reason for hiding this comment

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

HeaderSub is not enough to get all of the transaction status in particular AcceptedOnL1 since that is updated in a separate go routine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So header Recv here is more than a timer tick than taking the Tx status.
In other words:

  • I'm getting the current status of Tx (the way rpc method does)
  • I should check it again in a while to send an eventual update
    • I can wait some arbitrary time period
    • I can wait until next block which more likely brings some changes (but not in all cases as you mentioned)
  • I'm getting the current status of Tx (the way rpc method does)

@pnowosie pnowosie force-pushed the pnowosie/2209-subscribe-tx-status-ws branch from a552376 to 5150b5f Compare December 17, 2024 10:01
@pnowosie pnowosie force-pushed the pnowosie/2209-subscribe-tx-status-ws branch 3 times, most recently from badc257 to 713b244 Compare December 20, 2024 11:16
@pnowosie pnowosie force-pushed the pnowosie/2209-subscribe-tx-status-ws branch from 713b244 to 2d073dd Compare December 23, 2024 10:04
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.

Implement starknet_subscribeTransactionStatus websocket method
4 participants