thiagodeev/rpcv08-websocket

[thiagodeev]

#623

This PR aims to implement some RPC v0.8.0 updates to starknet.go. The ones addressed by this PR are:

New methods:
starknet_subscribeNewHeads
starknet_subscribeEvents
starknet_subscribeTransactionStatus
starknet_subscribePendingTransactions
starknet_unsubscribe

Components
NEW_TXN_STATUS
REORG_DATA
SUBSCRIPTION_BLOCK_ID

Errors:
INVALID_SUBSCRIPTION_ID
TOO_MANY_ADDRESSES_IN_FILTER
TOO_MANY_BLOCKS_BACK

In order to implement websocket connection with Starknet in starknet.go I had to make some adaptations to the client code being currently used, and to do that, I had to copy the entire client code.

[rianhughes]

suggestion

func (provider *WsProvider) SubscribeEvents(ctx context.Context, events chan<- *EmittedEvent, input EventSubscriptionInput) (*client.ClientSubscription, error) {
	if input.BlockID.IsZero() {
		input.BlockID = BlockID{Tag: "latest"}
	}
	sub, err := provider.c.Subscribe(ctx, "starknet", "_subscribeEvents", events, input)
	if err != nil {
		return nil, tryUnwrapToRPCErr(err, ErrTooManyKeysInFilter, ErrTooManyBlocksBack, ErrBlockNotFound, ErrCallOnPending)
	}
	return sub, nil
}

Also, add the IsZero() method

func (b BlockID) IsZero() bool {
	return b == BlockID{}
}

Maybe, also just make the default BlockID a global varialbe (in types_block.go) and use that instead (probably cleaner)

var	DefaultBlockID = BlockID{Tag: "latest"}

[thiagodeev]

Yeah, I know it defaults to "latest" even if we don't pass any blockID, but a json request with an empty param is different from a request passing values. The json will be different from the one requested by the user, even if the result is the same.
I just wanted to make the final json request exactly the one the user is creating in the code. WDYT?

[rianhughes]

Yeah that's a good point. Although, given an absent blockId is the same as the default value, I think overriding it is okay. Plus this approach is a bit neater, and less error prone imo

[thiagodeev]

Please take a look at my new approach and let me know your opinion

[rianhughes]

lgtm

[rianhughes]

Although, we seem to repeat the same logic multiple times in different ways, I feel like we should unify it.
Eg in SubscribeEvents we do

var emptyBlockID BlockID
	if options.BlockID == emptyBlockID {
		options.BlockID = WithBlockTag("latest")
	}

but in SubscribeNewHeads we have

if blockID == nil {
		blockID = &BlockID{Tag: "latest"}
	}

and again in SubscribeTransactionStatus

[thiagodeev]

The spec was updated. Take a look at my new approach

[rianhughes]

Imo this should always be executed if we had no error subscribing in the first place, so I would suggest using defer sub.Unsubscribe() between line 40 and 41

[rianhughes]

Also, I think we can simplify this example by removing the code below this line, and explicitly exit after we receive eg 5 blocks form the ws endpoint

[thiagodeev]

But each block takes ~15-20 seconds to build, so they'll need to wait almost 2 minutes for it.
We can indeed simplify it, but IMO the example should be both simple and complete. My intuition for including the last code part was to show the effects of the parameters and how to use them.
WDYT?

[rianhughes]

Do we need SubscribeWithSliceArgs? Can we not just update Subscribe to be
Subscribe(ctx context.Context, namespace string, methodSuffix string, channel interface{}, args ...interface{})

a quick check suggests it works

[thiagodeev]

The answer is that the client was passing the args as an array by default, and I was facing an issue with Juno related to optional parameters (it is not accepting to omit any parameters when passing as an array, even if they're optional), so I adapted the "Subscribe" function to send "any" instead of "[]any", and created another method, "SubscribeWithSliceArgs", to maintain compatibility with the remaining codebase and tests.
So when we need to have optional parameters, we need to use "Subscribe", but when all of them are needed, instead of creating a struct for it, we can just send them through "SubscribeWithSliceArgs"

[rianhughes]

I think the issue might have been related to not specifying the parameter names in the request, eg updating the SubscribeTransactionStatus method as follows seems to work

func (provider *WsProvider) SubscribeTransactionStatus(ctx context.Context, newStatus chan<- *NewTxnStatusResp, transactionHash QWEQWE) (*client.ClientSubscription, error) {
    sub, err := provider.c.Subscribe(ctx, "starknet", "_subscribeTransactionStatus", newStatus, transactionHash)
    if err != nil {
        return nil, tryUnwrapToRPCErr(err, ErrTooManyBlocksBack, ErrBlockNotFound)
    }
    return sub, nil
}

type QWEQWE struct {
    TransactionHash *felt.Felt `json:"transaction_hash"`
}

This explicitly sets the parameter names in the request, eg like

{"jsonrpc":"2.0","id":1,"method":"starknet_subscribeTransactionStatus","params":{"transaction_hash":"0x7b"}}

instead of something like this
{"jsonrpc":"2.0","id":1,"method":"starknet_subscribeTransactionStatus","params":["0x7b","0x7b"]}

when using SubscribeWithSliceArgs

[rianhughes]

The optional json tag should be able to handle the case where the parameter is optional as well

[thiagodeev]

Yes, you're right. But the current solution for the problem is:

• Subscribe was changed to receive any instead of []any
• A new method called SubscribeWithSliceArgs was created to perfectly replace the older version of the Subscribe method where it was used. This method simply takes an []any and passes it to Subscribe.
  It's simple and works well.

But if we remove the SubscribeWithSliceArgs, we'll need to wrap single values params in a struct only to make it work, we'll also need to create new structs inside the tests that came from ethrpc only to make it work. I don't see any advantages in doing this, only disadvantages.

WDYT?

[rianhughes]

What are you trying to check for here??

[thiagodeev]

I changed it a little bit. It's about testing whether events from different addresses and keys are being returned, as this testcase doesn't have any filter

[rianhughes]

I wonder if we could just do something like

require.EqualError(t,err,test.ExpectedErr)
if test.expectedError != nil { return}

[thiagodeev]

Unfortunately not with 'EqualError', because it '...asserts that a function returned an error (i.e. not nil)...' first.
I think your previous suggestion of removing the 'else' is enough

[rianhughes]

Note for myself to revisit this

[thiagodeev]

Did you see my comment?

#651 (comment)

[rianhughes]

Maybe we should add some mocks for the WebsocketProvider, although I would suggest doing this in a separate PR

[rianhughes]

It would allow us to run mock tests

[thiagodeev]

Do you want me to fix this right after this RP was merged?