From 8767ffadc0d51325c7cb6806cc62bcadd0941beb Mon Sep 17 00:00:00 2001 From: Jeff Schroeder Date: Mon, 10 Jul 2023 15:16:30 +0000 Subject: [PATCH] node: fix url handling of the value to --suiWS Fixes: #2827 Previously, it prepended `ws://` to the address unlike any of the other websocket flags. This allows specifying it the same was as guardiand v2.16.0 or like the rest. In the future, we can remove the "legacy" way and make them all consistent. --- node/pkg/watchers/sui/watcher.go | 29 +++++++-- node/pkg/watchers/sui/watcher_test.go | 84 +++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 6 deletions(-) create mode 100644 node/pkg/watchers/sui/watcher_test.go diff --git a/node/pkg/watchers/sui/watcher.go b/node/pkg/watchers/sui/watcher.go index f8b9ef0da5..a9a6a6ebdf 100644 --- a/node/pkg/watchers/sui/watcher.go +++ b/node/pkg/watchers/sui/watcher.go @@ -267,6 +267,13 @@ func (e *Watcher) Run(ctx context.Context) error { logger := supervisor.Logger(ctx) + // guardiand v2.16.0 shipped hardcoding "ws://" for the websocket url. This makes + // the flag value the same as all of the other uses of rpc websocket values. + err := e.fixSuiWsURL(logger) + if err != nil { + return err + } + logger.Info("Starting watcher", zap.String("watcher_name", "sui"), zap.String("suiRPC", e.suiRPC), @@ -275,12 +282,7 @@ func (e *Watcher) Run(ctx context.Context) error { zap.Bool("unsafeDevMode", e.unsafeDevMode), ) - u := url.URL{Scheme: "ws", Host: e.suiWS} - - logger.Info("Sui watcher connecting to WS node ", zap.String("url", u.String())) - logger.Debug("SUI watcher:", zap.String("suiRPC", e.suiRPC), zap.String("suiWS", e.suiWS), zap.String("suiMoveEventType", e.suiMoveEventType)) - - ws, _, err := websocket.Dial(ctx, u.String(), nil) + ws, _, err := websocket.Dial(ctx, e.suiWS, nil) if err != nil { p2p.DefaultRegistry.AddErrorCount(vaa.ChainIDSui, 1) suiConnectionErrors.WithLabelValues("websocket_dial_error").Inc() @@ -498,3 +500,18 @@ func (e *Watcher) Run(ctx context.Context) error { return err } } + +// fixSuiWsURL ensures the websocket scheme is properly specified +func (e *Watcher) fixSuiWsURL(logger *zap.Logger) error { + u, _ := url.Parse(e.suiWS) + + // When the scheme is empty/nil or when the Host is empty but a scheme eis set + if u == nil || u.Scheme == "" || (u.Scheme != "" && u.Host == "") { + logger.Warn(fmt.Sprintf("DEPRECATED: Prefix --suiWS address with the url scheme e.g.: ws://%s or wss://%s", e.suiWS, e.suiWS)) + u = &url.URL{Scheme: "ws", Host: e.suiWS} + } else if u.Scheme != "ws" && u.Scheme != "wss" { + return fmt.Errorf("invalid url scheme specified for --suiWS, try ws:// or wss://: %s", e.suiWS) + } + e.suiWS = u.String() + return nil +} diff --git a/node/pkg/watchers/sui/watcher_test.go b/node/pkg/watchers/sui/watcher_test.go new file mode 100644 index 0000000000..983f4897b6 --- /dev/null +++ b/node/pkg/watchers/sui/watcher_test.go @@ -0,0 +1,84 @@ +package sui + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "go.uber.org/zap/zaptest/observer" +) + +func Test_fixSuiWsURL(t *testing.T) { + tests := []struct { + name string + value string + expected string + logMessage string + errMessage string + }{ + { + name: "valid", + value: "ws://1.2.3.4:5678", + expected: "ws://1.2.3.4:5678", + }, + { + name: "tilt", + value: "sui:9000", + expected: "ws://sui:9000", + logMessage: "DEPRECATED: Prefix --suiWS address with the url scheme e.g.: ws://sui:9000 or wss://sui:9000", + }, + { + name: "valid-no-port", + value: "ws://1.2.3.4", + expected: "ws://1.2.3.4", + }, + { + name: "no-scheme", + value: "1.2.3.4:5678", + expected: "ws://1.2.3.4:5678", + logMessage: "DEPRECATED: Prefix --suiWS address with the url scheme e.g.: ws://1.2.3.4:5678 or wss://1.2.3.4:5678", + }, + { + name: "no-scheme-no-port", + value: "1.2.3.4", + expected: "ws://1.2.3.4", + logMessage: "DEPRECATED: Prefix --suiWS address with the url scheme e.g.: ws://1.2.3.4 or wss://1.2.3.4", + }, + { + name: "wrong-scheme", + value: "http://1.2.3.4", + errMessage: "invalid url scheme specified for --suiWS, try ws:// or wss://: http://1.2.3.4", + }, + } + for _, testCase := range tests { + t.Run(testCase.name, func(t *testing.T) { + testCore, logs := observer.New(zap.InfoLevel) + testLogger := zap.New(testCore) + + suiWatcher := Watcher{ + suiWS: testCase.value, + } + err := suiWatcher.fixSuiWsURL(testLogger) + if testCase.errMessage != "" { + require.EqualError(t, err, testCase.errMessage) + } else { + require.NoError(t, err) + // Only verify the value if no error was returned + assert.Equal(t, testCase.expected, suiWatcher.suiWS) + } + + if len(testCase.logMessage) != 0 { + // If the testcase expects a log, then there should only be 1 log + require.Equal(t, 1, logs.Len()) + + // Ensure the log message is correct + actualLogMessage := logs.All()[0].Message + require.Equal(t, testCase.logMessage, actualLogMessage) + } else { + // If the testcase does not expect a log, none should be emitted + require.Equal(t, 0, logs.Len()) + } + }) + } +}