From 7ce826662e98e44a07e65033b61b6efd94326bbc Mon Sep 17 00:00:00 2001 From: Ze Date: Sat, 5 Oct 2024 11:29:35 -0700 Subject: [PATCH] refactor(panic): remove unused or unnecessary panic code --- client/x/evmstaking/types/params.go | 24 ------ client/x/evmstaking/types/params_test.go | 82 -------------------- client/x/evmstaking/types/withdraw.go | 29 -------- client/x/evmstaking/types/withdraw_test.go | 87 ---------------------- lib/errors/errors.go | 2 +- lib/promutil/resetgauge.go | 71 ------------------ lib/promutil/resetgauge_test.go | 76 ------------------- 7 files changed, 1 insertion(+), 370 deletions(-) delete mode 100644 lib/promutil/resetgauge.go delete mode 100644 lib/promutil/resetgauge_test.go diff --git a/client/x/evmstaking/types/params.go b/client/x/evmstaking/types/params.go index b9a8e6ae..bdefd22c 100644 --- a/client/x/evmstaking/types/params.go +++ b/client/x/evmstaking/types/params.go @@ -2,10 +2,6 @@ package types import ( "fmt" - - "github.com/cosmos/cosmos-sdk/codec" - - "github.com/piplabs/story/lib/errors" ) // Staking params default values. @@ -35,26 +31,6 @@ func DefaultParams() Params { ) } -// unmarshal the current params value from store key or panic. -func MustUnmarshalParams(cdc *codec.LegacyAmino, value []byte) Params { - params, err := UnmarshalParams(cdc, value) - if err != nil { - panic(err) - } - - return params -} - -// unmarshal the current params value from store key. -func UnmarshalParams(cdc *codec.LegacyAmino, value []byte) (params Params, err error) { - err = cdc.Unmarshal(value, ¶ms) - if err != nil { - return params, errors.Wrap(err, "unmarshal params") - } - - return params, nil -} - func ValidateMaxWithdrawalPerBlock(v uint32) error { if v == 0 { return fmt.Errorf("max withdrawal per block must be positive: %d", v) diff --git a/client/x/evmstaking/types/params_test.go b/client/x/evmstaking/types/params_test.go index dab1c8cd..5c7019eb 100644 --- a/client/x/evmstaking/types/params_test.go +++ b/client/x/evmstaking/types/params_test.go @@ -37,88 +37,6 @@ func (suite *ParamsTestSuite) TestDefaultParams() { require.Equal(types.DefaultMinPartialWithdrawalAmount, params.MinPartialWithdrawalAmount) } -func (suite *ParamsTestSuite) TestMustUnmarshalParams() { - require := suite.Require() - maxWithdrawalPerBlock, maxSweepPerBlock, minPartialWithdrawalAmount := uint32(1), uint32(2), uint64(3) - params := types.NewParams(maxWithdrawalPerBlock, maxSweepPerBlock, minPartialWithdrawalAmount) - - tcs := []struct { - name string - input []byte - expected types.Params - expectPanic bool - }{ - { - name: "Unmarshal valid params bytes", - input: suite.encConf.Codec.MustMarshal(¶ms), - expected: types.Params{ - MaxWithdrawalPerBlock: maxWithdrawalPerBlock, - MaxSweepPerBlock: maxSweepPerBlock, - MinPartialWithdrawalAmount: minPartialWithdrawalAmount, - }, - }, - { - name: "Unmarshal invalid params bytes", - input: []byte{0x1, 0x2, 0x3}, - expectPanic: true, - }, - } - - for _, tc := range tcs { - suite.Run(tc.name, func() { - if tc.expectPanic { - require.Panics(func() { - types.MustUnmarshalParams(suite.encConf.Amino, tc.input) - }) - } else { - params := types.MustUnmarshalParams(suite.encConf.Amino, tc.input) - require.Equal(tc.expected, params) - } - }) - } -} - -func (suite *ParamsTestSuite) TestUnmarshalParams() { - require := suite.Require() - maxWithdrawalPerBlock, maxSweepPerBlock, minPartialWithdrawalAmount := uint32(1), uint32(2), uint64(3) - params := types.NewParams(maxWithdrawalPerBlock, maxSweepPerBlock, minPartialWithdrawalAmount) - - tcs := []struct { - name string - input []byte - expected types.Params - expectedError string - }{ - { - name: "Unmarshal valid params bytes", - input: suite.encConf.Codec.MustMarshal(¶ms), - expected: types.Params{ - MaxWithdrawalPerBlock: maxWithdrawalPerBlock, - MaxSweepPerBlock: maxSweepPerBlock, - MinPartialWithdrawalAmount: minPartialWithdrawalAmount, - }, - }, - { - name: "Unmarshal invalid params bytes", - input: []byte{0x1, 0x2, 0x3}, - expectedError: "unmarshal params", - }, - } - - for _, tc := range tcs { - suite.Run(tc.name, func() { - params, err := types.UnmarshalParams(suite.encConf.Amino, tc.input) - if tc.expectedError != "" { - require.Error(err) - require.Contains(err.Error(), tc.expectedError) - } else { - require.NoError(err) - require.Equal(tc.expected, params) - } - }) - } -} - func (suite *ParamsTestSuite) TestValidateMaxWithdrawalPerBlock() { require := suite.Require() diff --git a/client/x/evmstaking/types/withdraw.go b/client/x/evmstaking/types/withdraw.go index 00bc1994..60893a4b 100644 --- a/client/x/evmstaking/types/withdraw.go +++ b/client/x/evmstaking/types/withdraw.go @@ -4,10 +4,6 @@ import ( "strings" "cosmossdk.io/core/address" - - "github.com/cosmos/cosmos-sdk/codec" - - "github.com/piplabs/story/lib/errors" ) // Withdrawals is a collection of Withdrawal. @@ -48,28 +44,3 @@ func NewWithdrawalFromMsg(msg *MsgAddWithdrawal) Withdrawal { Amount: msg.Withdrawal.Amount, } } - -func MustMarshalWithdrawal(cdc codec.BinaryCodec, withdrawal *Withdrawal) []byte { - return cdc.MustMarshal(withdrawal) -} - -// MustUnmarshalWithdrawal return the unmarshaled withdrawal from bytes. -// Panics if fails. -func MustUnmarshalWithdrawal(cdc codec.BinaryCodec, value []byte) Withdrawal { - withdrawal, err := UnmarshalWithdrawal(cdc, value) - if err != nil { - panic(err) - } - - return withdrawal -} - -// UnmarshalWithdrawal returns the withdrawal. -func UnmarshalWithdrawal(cdc codec.BinaryCodec, value []byte) (withdrawal Withdrawal, err error) { - err = cdc.Unmarshal(value, &withdrawal) - if err != nil { - return withdrawal, errors.Wrap(err, "unmarshal withdrawal") - } - - return withdrawal, nil -} diff --git a/client/x/evmstaking/types/withdraw_test.go b/client/x/evmstaking/types/withdraw_test.go index db181212..29f873b4 100644 --- a/client/x/evmstaking/types/withdraw_test.go +++ b/client/x/evmstaking/types/withdraw_test.go @@ -109,93 +109,6 @@ func (suite *WithdrawTestSuite) TestNewWithdrawalFromMsg() { require.Equal(withdrawal, w, "NewWithdrawalFromMsg should return the same withdrawal") } -func (suite *WithdrawTestSuite) TestMustMarshalWithdraw() { - require := suite.Require() - withdrawal := types.NewWithdrawal(1, suite.delAddr, suite.valAddr, suite.evmAddr.String(), 1) - require.NotPanics(func() { - marshaled := types.MustMarshalWithdrawal(suite.encConf.Codec, &withdrawal) - require.NotNil(marshaled, "MarshalWithdrawal should not return nil") - }) -} - -func (suite *WithdrawTestSuite) TestUnmarshalWithdraw() { - require := suite.Require() - withdrawal := types.NewWithdrawal(1, suite.delAddr, suite.valAddr, suite.evmAddr.String(), 1) - - testCases := []struct { - name string - input []byte - expectedResult types.Withdrawal - expectError bool - }{ - { - name: "Unmarshal valid withdrawal bytes", - input: types.MustMarshalWithdrawal(suite.encConf.Codec, &withdrawal), - expectedResult: types.NewWithdrawal(1, suite.delAddr, suite.valAddr, suite.evmAddr.String(), 1), - expectError: false, - }, - { - name: "Unmarshal invalid withdrawal bytes", - input: []byte{1}, - expectedResult: types.Withdrawal{}, // Expecting an empty struct since it will fail - expectError: true, - }, - } - - // Iterate over test cases - for _, tc := range testCases { - suite.Run(tc.name, func() { - result, err := types.UnmarshalWithdrawal(suite.encConf.Codec, tc.input) - if tc.expectError { - require.Error(err) - } else { - require.NoError(err) - require.Equal(tc.expectedResult, result, "UnmarshalWithdrawal should return the correct withdrawal") - } - }) - } -} - -func (suite *WithdrawTestSuite) TestMustUnmarshalWithdraw() { - require := suite.Require() - withdrawal := types.NewWithdrawal(1, suite.delAddr, suite.valAddr, suite.evmAddr.String(), 1) - - testCases := []struct { - name string - input []byte - expected types.Withdrawal - expectPanic bool - }{ - { - name: "Unmarshal valid withdrawal bytes", - input: types.MustMarshalWithdrawal(suite.encConf.Codec, &withdrawal), - expected: types.NewWithdrawal(1, suite.delAddr, suite.valAddr, suite.evmAddr.String(), 1), - expectPanic: false, - }, - { - name: "Unmarshal invalid withdrawal bytes - panic", - input: []byte{1}, - expectPanic: true, - }, - } - - // Iterate over test cases - for _, tc := range testCases { - suite.Run(tc.name, func() { - if tc.expectPanic { - require.Panics(func() { - types.MustUnmarshalWithdrawal(suite.encConf.Codec, tc.input) - }) - } else { - require.NotPanics(func() { - result := types.MustUnmarshalWithdrawal(suite.encConf.Codec, tc.input) - require.Equal(tc.expected, result, "MustUnmarshalWithdrawal should return the correct withdrawal") - }) - } - }) - } -} - func TestWithdrawalTestSuite(t *testing.T) { t.Parallel() suite.Run(t, new(WithdrawTestSuite)) diff --git a/lib/errors/errors.go b/lib/errors/errors.go index 842b1ec2..3eb62630 100644 --- a/lib/errors/errors.go +++ b/lib/errors/errors.go @@ -22,7 +22,7 @@ func New(msg string, attrs ...any) error { //nolint:inamedparam // This function does custom wrapping and errors. func Wrap(err error, msg string, attrs ...any) error { if err == nil { - panic("wrap nil error") + return nil } // Support error types that do their own wrapping. diff --git a/lib/promutil/resetgauge.go b/lib/promutil/resetgauge.go deleted file mode 100644 index 17706087..00000000 --- a/lib/promutil/resetgauge.go +++ /dev/null @@ -1,71 +0,0 @@ -// Copyright © 2022-2023 Obol Labs Inc. Licensed under the terms of a Business Source License 1.1 - -// Package promutil provides Prometheus utilities. -// This was copied from Obol's Charon repo. -package promutil - -import ( - "strings" - "sync" - - "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/promauto" -) - -const separator = "|" - -// NewResetGaugeVec creates a new ResetGaugeVec. -func NewResetGaugeVec(opts prometheus.GaugeOpts, labelNames []string) *ResetGaugeVec { - return &ResetGaugeVec{ - inner: promauto.NewGaugeVec(opts, labelNames), - labels: make(map[string]bool), - } -} - -// ResetGaugeVec is a GaugeVec that can be reset which deletes all previously set labels. -// This is useful to clear out labels that are no longer present. -type ResetGaugeVec struct { - inner *prometheus.GaugeVec - - mu sync.Mutex - labels map[string]bool -} - -func (g *ResetGaugeVec) WithLabelValues(lvs ...string) prometheus.Gauge { - for _, lv := range lvs { - if strings.Contains(lv, separator) { - panic("label value cannot contain separator") - } - } - - g.mu.Lock() - defer g.mu.Unlock() - - g.labels[strings.Join(lvs, separator)] = true - - return g.inner.WithLabelValues(lvs...) -} - -// Reset deletes all previously set labels that match all the given label values. -// An empty slice will delete all previously set labels. -func (g *ResetGaugeVec) Reset(lvs ...string) { - g.mu.Lock() - defer g.mu.Unlock() - - for label := range g.labels { - match := true - for _, check := range lvs { - if !strings.Contains(label, check) { - match = false - break - } - } - - if !match { - continue - } - - g.inner.DeleteLabelValues(strings.Split(label, separator)...) - delete(g.labels, label) - } -} diff --git a/lib/promutil/resetgauge_test.go b/lib/promutil/resetgauge_test.go deleted file mode 100644 index f6450509..00000000 --- a/lib/promutil/resetgauge_test.go +++ /dev/null @@ -1,76 +0,0 @@ -// Copyright © 2022-2023 Obol Labs Inc. Licensed under the terms of a Business Source License 1.1 - -package promutil_test - -import ( - "testing" - - "github.com/prometheus/client_golang/prometheus" - "github.com/stretchr/testify/require" - - "github.com/piplabs/story/lib/promutil" -) - -//nolint:paralleltest // This test uses global prometheus registry so concurrent tests are not safe. -func TestResetGaugeVec(t *testing.T) { - const resetTest = "reset_test" - - var testResetGauge = promutil.NewResetGaugeVec(prometheus.GaugeOpts{ - Name: resetTest, - Help: "", - }, []string{"label0", "label1"}) - - testResetGauge.WithLabelValues("1", "a").Set(0) - assertVecLen(t, resetTest, 1) - - // Same labels, should not increase length - testResetGauge.WithLabelValues("1", "a").Set(1) - assertVecLen(t, resetTest, 1) - - testResetGauge.WithLabelValues("2", "b").Set(2) - assertVecLen(t, resetTest, 2) - - testResetGauge.Reset() - assertVecLen(t, resetTest, 0) - - testResetGauge.WithLabelValues("3", "c").Set(3) - assertVecLen(t, resetTest, 1) - - testResetGauge.WithLabelValues("3", "d").Set(3) - assertVecLen(t, resetTest, 2) - - testResetGauge.WithLabelValues("3", "e").Set(3) - assertVecLen(t, resetTest, 3) - - testResetGauge.WithLabelValues("4", "z").Set(4) - assertVecLen(t, resetTest, 4) - - testResetGauge.Reset("3", "c") - assertVecLen(t, resetTest, 3) - - testResetGauge.Reset("3") - assertVecLen(t, resetTest, 1) -} - -func assertVecLen(t *testing.T, name string, l int) { //nolint:unparam // abstracting name is fine even though it is always currently constant - t.Helper() - - metrics, err := prometheus.DefaultGatherer.Gather() - require.NoError(t, err) - - for _, metricFam := range metrics { - if metricFam.GetName() != name { - continue - } - - require.Len(t, metricFam.GetMetric(), l) - - return - } - - if l == 0 { - return - } - - require.Fail(t, "metric not found") -}