WS URL can be optional when LogBroadcaster is disabled #14364
Conversation
huangzhen1997
requested review from
reductionista and
dhaidashenko
and removed request for
a team
core/chains/evm/client/evm_client.go
Outdated
| @@ -20,7 +20,7 @@ func NewEvmClient(cfg evmconfig.NodePool, chainCfg commonclient.ChainConfig, cli | |||
| var sendonlys []commonclient.SendOnlyNode[*big.Int, RPCClient] | |||
| largePayloadRPCTimeout, defaultRPCTimeout := getRPCTimeouts(chainType) | |||
| for i, node := range nodes { | |||
| if node.SendOnly != nil && *node.SendOnly { | |||
| if node.WSURL == nil || (node.SendOnly != nil && *node.SendOnly) { | |||
There was a problem hiding this comment.
A secondary or SendOnly node has limited functionality. It's only used to broadcast transactions (fire and forget), and we are not running health checks for it.
We should not change the type of the node. Instead, introduce changes to the RPC client.
There was a problem hiding this comment.
Moving the enforcement check to Dial() at rpc_client.go
…BCFR-451/make-ws-url-optional
| } | ||
|
|
||
| if !hasPrimary { | ||
| err = multierr.Append(err, commonconfig.ErrMissing{Name: "Nodes", | ||
| Msg: "must have at least one primary node with WSURL"}) |
There was a problem hiding this comment.
| Msg: "must have at least one primary node with WSURL"}) | |
| Msg: "must have at least one primary node"}) |
|
|
||
| // if the node is a primary node, then the WS URL is required when LogBroadcaster is enabled | ||
| if logBroadcasterEnabled && (n.WSURL == nil || n.WSURL.IsZero()) { | ||
| err = multierr.Append(err, commonconfig.ErrMissing{Name: "Nodes", Msg: fmt.Sprintf("%vth node is a primary node and it's missing valid WSURL with LogBroadcaster is enabled", i)}) |
There was a problem hiding this comment.
| err = multierr.Append(err, commonconfig.ErrMissing{Name: "Nodes", Msg: fmt.Sprintf("%vth node is a primary node and it's missing valid WSURL with LogBroadcaster is enabled", i)}) | |
| err = multierr.Append(err, commonconfig.ErrMissing{Name: "Nodes", Msg: fmt.Sprintf("%vth primary node must have a valid WSURL when LogBroadcaster is enabled", i)}) |
There was a problem hiding this comment.
%vth node instead of %vth primary node is more accurate. Or %vth node (primary)
core/chains/evm/client/evm_client.go
Outdated
| if node.SendOnly != nil && *node.SendOnly { | ||
| rpc := NewRPCClient(lggr, empty, (*url.URL)(node.HTTPURL), *node.Name, int32(i), chainID, | ||
| rpc := NewRPCClient(lggr, ws, (*url.URL)(node.HTTPURL), *node.Name, int32(i), chainID, |
There was a problem hiding this comment.
| rpc := NewRPCClient(lggr, ws, (*url.URL)(node.HTTPURL), *node.Name, int32(i), chainID, | |
| rpc := NewRPCClient(lggr, nil, (*url.URL)(node.HTTPURL), *node.Name, int32(i), chainID, |
Previously, we always passed empty URLs for sendOnly RPC clients. To avoid unexpected behavior, we should pass nil instead of the URL specified in the config.
In theory, NOPs might have specified unreachable URL/empty URL, which we've previously ignored.
But with this change and changes from Dylans PR will start using it, which could cause issue.
|
Description
WS URL can be optional (empty string) when LogBroadcaster is disabled. If WS URL was not provided, SubscribeFilterLogs should fail with an explicit error
Tickets:
BCFR-451