-
Notifications
You must be signed in to change notification settings - Fork 114
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
thiagodeev/rpcv08-websocket #651
base: v0.8.0
Are you sure you want to change the base?
Conversation
61eb5fb
to
9199306
Compare
…ith Websocket support - Added gorilla/websocket v1.5.3 as a direct dependency in go.mod. - Introduced NewWebsocketProvider function in provider.go to create a Websocket RPC Provider instance, enhancing the existing HTTP provider functionality.
- Removed the dependency on github.com/ethereum/go-ethereum and replaced it with github.com/NethermindEth/starknet.go/client in go.mod and related files. - Introduced new WebSocket provider functionality in provider.go
…od to work on starknet
- Updated the handler to correctly process subscription IDs for Starknet, accommodating the new structure returned by the Starknet API. - Modified the subscriptionResult struct to include a Starknet-specific subscription ID field. - Adjusted the WebSocket provider to support new block header subscriptions, improving the overall subscription mechanism. - Updated example usage to reflect changes in subscription handling and error management. These changes improve compatibility with Starknet's API and enhance the robustness of the WebSocket client.
- Updated the main CI workflow to include testing for the client with mocks. - Modified subscription handling in `subscription.js` to accommodate the new `subscription_id` structure from Starknet. - Refactored `provider_test.go` to include WebSocket provider support and improved test configurations. - Introduced a new test file `websocket_test.go` to validate WebSocket subscriptions for new block headers, ensuring robust error handling and compatibility with the testnet environment. These changes improve the overall robustness of the RPC client and enhance compatibility with Starknet's API.
f846bb8
to
05d7e90
Compare
4bb568c
to
c73bfce
Compare
Note: at the moment it's not possible to omit optional parameters in json-rpc calls using array as structure type with Juno, and the current client implementation only supports sending parameters as arrays. Therefore, I changed the Subscribe function and now we are able to send optional parameters as object too. That way Juno doesn't return an error
…hanced error handling and testing - Added the SubscribeEvents method to the WebSocket provider, allowing for event subscriptions with optional parameters. - Introduced a new test case in websocket_test.go to validate event subscriptions under various conditions, including handling of empty arguments and specific address filtering. - Updated the HexToFeltNoErr utility function to convert hexadecimal strings to felt objects without error handling, suitable for internal use. These changes improve the robustness of the WebSocket client and enhance compatibility with Starknet's event subscription model.
// HexToFelt converts a hexadecimal string to a *felt.Felt object, ignoring errors. | ||
// | ||
// Note: only use this function if you are sure that the input is a valid felt input. | ||
// Not recommended for production use. Always handle errors correctly. | ||
// | ||
// Parameters: | ||
// - hex: the input hexadecimal string to be converted. | ||
// Returns: | ||
// - *felt.Felt: a *felt.Felt object | ||
func HexToFeltNoErr(hex string) *felt.Felt { | ||
felt, _ := new(felt.Felt).SetString(hex) | ||
return felt | ||
} |
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.
Given this is only used in tests we could do something like this,
func HexToFelt(t testing.TB, hex string) *felt.Felt {
t.Helper()
f, err := new(felt.Felt).SetString(hex)
require.NoError(t, err)
return f
}
In general I would advise against ignoring errors in functions like this
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.
Me too.
Also, for tests there's already the TestHexToFelt
func that does exactly what you mentioned.
I thought about this function when we need to put some values that we have sure are valid felts (like addresses, hashes...) into (for e.g.) an object, and as the felt (for good reasons) returns an error in such operations we need to declare and set the variable previously. With this function we can set it directly. WDYT?
type WsProvider struct { | ||
c wsConn | ||
} | ||
|
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.
We should probably implement an interface defining the set of functions that the websocket provider must implement, basically something like
var _ RpcProvider = &Provider{}
for the websocket provider
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.
Agree.
It is in my plans
fmt.Println("Starting simpleCall example") | ||
|
||
// Initialize connection to RPC provider | ||
client, err := rpc.NewWebsocketProvider("ws://localhost:6061") //local juno node for testing |
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.
We should set this as an ENV variable to be consistent with the other examples
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.
Agree.
I'm currently using this file to test each endpoint I'm creating, but I'm not committing the changes.
Once I finish the features I'll make a proper example for websocket
examples/websocket/main.go
Outdated
sub, err := client.SubscribeNewHeads(context.Background(), ch) | ||
if err != nil { | ||
rpcErr := err.(*rpc.RPCError) | ||
panic(fmt.Sprintf("Error subscribing: %s", rpcErr.Error())) | ||
} | ||
|
||
for { |
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 there was no error, we should unsubscribe when exiting main,
defer sub.Unsubscribe()
This example calls two contract functions, with and without calldata. It uses an ERC20 token, but it can be any smart contract. | ||
|
||
Steps: | ||
1. Rename the ".env.template" file located at the root of the "examples" folder to ".env" | ||
1. Uncomment, and assign your Sepolia testnet endpoint to the `RPC_PROVIDER_URL` variable in the ".env" file | ||
1. Uncomment, and assign your account address to the `ACCOUNT_ADDRESS` variable in the ".env" file | ||
1. Make sure you are in the "simpleCall" directory | ||
1. Execute `go run main.go` | ||
|
||
The calls outuputs will be returned at the end of the execution. |
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 this README needs updated
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.
Yes. I'll do this at the end
rpc/websocket.go
Outdated
func (provider *WsProvider) SubscribeEvents(ctx context.Context, events chan<- *EmittedEvent, input EventSubscriptionInput) (*client.ClientSubscription, error) { | ||
var sub *client.ClientSubscription | ||
var err error | ||
|
||
var emptyBlockID BlockID | ||
if input.BlockID == emptyBlockID { | ||
// BlockID has a custom MarshalJSON that doesn't allow zero values. | ||
// Create a temporary struct without BlockID field to properly handle the optional parameter. | ||
tempInput := struct { | ||
FromAddress *felt.Felt `json:"from_address,omitempty"` | ||
Keys [][]*felt.Felt `json:"keys,omitempty"` | ||
}{ | ||
FromAddress: input.FromAddress, | ||
Keys: input.Keys, | ||
} | ||
|
||
sub, err = provider.c.Subscribe(ctx, "starknet", "_subscribeEvents", events, tempInput) | ||
} else { | ||
sub, err = provider.c.Subscribe(ctx, "starknet", "_subscribeEvents", events, input) | ||
} | ||
|
||
if err != nil { | ||
return nil, tryUnwrapToRPCErr(err, ErrTooManyKeysInFilter, ErrTooManyBlocksBack, ErrBlockNotFound, ErrCallOnPending) | ||
} | ||
return sub, nil | ||
} |
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.
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"}
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.
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?
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.
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
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.
Please take a look at my new approach and let me know your opinion
// TODO: wait for Juno to implement this. This is the correct implementation by the spec | ||
// sub, err := provider.c.SubscribeWithSliceArgs(ctx, "starknet", "_subscribeTransactionStatus", newStatus, transactionHash) | ||
// if err != nil { | ||
// return nil, tryUnwrapToRPCErr(err) | ||
// } | ||
return sub, nil |
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 just return a "waiting for Juno to implement this method" error?
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.
Because it'll be easier for me at the end to just uncomment the correct code and delete the other hahaha.
Hope you don't mind. I'll fix it anyway at the end
// - clientSubscription: The client subscription object, used to unsubscribe from the stream and to get errors | ||
// - error: An error, if any | ||
func (provider *WsProvider) SubscribeTransactionStatus(ctx context.Context, newStatus chan<- *NewTxnStatusResp, transactionHash *felt.Felt) (*client.ClientSubscription, error) { | ||
sub, err := provider.c.SubscribeWithSliceArgs(ctx, "starknet", "_subscribeTransactionStatus", newStatus, transactionHash, WithBlockTag("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.
Why do we pass in the BlockID? Also, maybe we can just use a global BlockID as mentioned in the above comment?
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 field is no longer accepted by this method according to the spec, but Juno hasn't implemented it yet when I wrote this. So I made this change to make it work normally and test it, but I'll revert it later
38a9150
to
c5c147f
Compare
@@ -255,6 +265,12 @@ func (sub *ClientSubscription) Err() <-chan error { | |||
return sub.err | |||
} | |||
|
|||
// Reorg returns a channel that notifies the subscriber of a reorganization of the chain. | |||
// A reorg event could be received only from subscribing to NewHeads, Events, and TransactionStatus | |||
func (sub *ClientSubscription) Reorg() <-chan *ReorgEvent { |
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.
The Reorg notification was tested locally using a Juno node with a small modification.
To simulate reorg notifications, I changed the Juno code so that instead of waiting for the feeder to return the Reorg event, it'll send one every 5 seconds, along with the main subscription.
So, I subscribed to the SubscribeNewHeads
method and the code handled perfectly both the main subscription and the ReorgEvents sents.
No description provided.