Skip to content
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.

Sign up for GitHub

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

chore: fix linter warnings #3311

Merged
merged 6 commits into from
Mar 22, 2023
Merged

chore: fix linter warnings #3311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fe8a679
chore: fix linter warnings
Mar 20, 2023
4444563
fix typos
Mar 20, 2023
c48c02c
Merge branch 'main' into carlos/fix-linter-warnings
Mar 21, 2023
2438e95
Merge branch 'main' into carlos/fix-linter-warnings
Mar 21, 2023
2fd14d3
fix golangci-lint version
Mar 22, 2023
f4bcec4
Merge branch 'main' into carlos/fix-linter-warnings
Mar 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,5 @@ jobs:
- name: golangci-lint
uses: golangci/[email protected]
with:
version: latest
version: v1.52.0
args: --timeout 5m
3 changes: 3 additions & 0 deletions .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ linters:

issues:
exclude-rules:
- text: "unused-parameter"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This linter rule complains when a named argument in a functions is not used

linters:
- revive
- text: "SA1019:"
linters:
- staticcheck
Expand Down
6 changes: 1 addition & 5 deletions modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,7 @@ func SetupICAPath(path *ibctesting.Path, owner string) error {
return err
}

if err := path.EndpointB.ChanOpenConfirm(); err != nil {
return err
}

return nil
return path.EndpointB.ChanOpenConfirm()
Copy link
Member

@damiannolan damiannolan Mar 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so an update to the linter started to trigger all these?
sometimes I prefer an explicit function call, and there may be scenarios where we may want to log something.

Happy to honour the linter here, just playing devils advocate. I think sometimes linters can be overly aggressive. We can always tweak the config later I guess.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be beneficial to add //nolint directives in that case with an additional comment for context or is that too noisy? I can definitely remember cases where a linter was being silly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, looks like golangci-lint was bumped to v1.52 and hell broke loose...

I had second thoughts when I was changing these as well, but thought the improvement was justified. I don't think the link would have complained if there was some extra logic inside the if statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be beneficial to add //nolint directives in that case with an additional comment for context or is that too noisy? I can definitely remember cases where a linter was being silly.

Yes, thank you for the suggestion, @DimitrisJim, we do use those directives in some places of the codebase.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @damiannolan, I think it'd be nice to be able to turn off this specific linting check. I don't see a lot of value in enforcing it, but I do see value in not enforcing it

}

func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
Expand Down
4 changes: 2 additions & 2 deletions modules/apps/27-interchain-accounts/controller/keeper/account.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ func (k Keeper) registerInterchainAccount(ctx sdk.Context, connectionID, portID,
case k.portKeeper.IsBound(ctx, portID) && !k.HasCapability(ctx, portID):
return "", errorsmod.Wrapf(icatypes.ErrPortAlreadyBound, "another module has claimed capability for and bound port with portID: %s", portID)
case !k.portKeeper.IsBound(ctx, portID):
cap := k.BindPort(ctx, portID)
if err := k.ClaimCapability(ctx, cap, host.PortPath(portID)); err != nil {
capability := k.BindPort(ctx, portID)
if err := k.ClaimCapability(ctx, capability, host.PortPath(portID)); err != nil {
return "", errorsmod.Wrapf(err, "unable to bind to newly generated portID: %s", portID)
}
}
Expand Down
4 changes: 2 additions & 2 deletions modules/apps/27-interchain-accounts/controller/keeper/account_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ func (suite *KeeperTestSuite) TestRegisterInterchainAccount() {
{
"port is already bound for owner but capability is claimed by another module",
func() {
cap := suite.chainA.GetSimApp().IBCKeeper.PortKeeper.BindPort(suite.chainA.GetContext(), TestPortID)
err := suite.chainA.GetSimApp().TransferKeeper.ClaimCapability(suite.chainA.GetContext(), cap, host.PortPath(TestPortID))
capability := suite.chainA.GetSimApp().IBCKeeper.PortKeeper.BindPort(suite.chainA.GetContext(), TestPortID)
err := suite.chainA.GetSimApp().TransferKeeper.ClaimCapability(suite.chainA.GetContext(), capability, host.PortPath(TestPortID))
suite.Require().NoError(err)
},
false,
Expand Down
4 changes: 2 additions & 2 deletions modules/apps/27-interchain-accounts/controller/keeper/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ import (
func InitGenesis(ctx sdk.Context, keeper Keeper, state genesistypes.ControllerGenesisState) {
for _, portID := range state.Ports {
if !keeper.HasCapability(ctx, portID) {
cap := keeper.BindPort(ctx, portID)
if err := keeper.ClaimCapability(ctx, cap, host.PortPath(portID)); err != nil {
capability := keeper.BindPort(ctx, portID)
if err := keeper.ClaimCapability(ctx, capability, host.PortPath(portID)); err != nil {
panic(fmt.Sprintf("could not claim port capability: %v", err))
}
}
Expand Down
6 changes: 1 addition & 5 deletions modules/apps/27-interchain-accounts/controller/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,7 @@ func SetupICAPath(path *ibctesting.Path, owner string) error {
return err
}

if err := path.EndpointB.ChanOpenConfirm(); err != nil {
return err
}

return nil
return path.EndpointB.ChanOpenConfirm()
}

// RegisterInterchainAccount is a helper function for starting the channel handshake
Expand Down
56 changes: 26 additions & 30 deletions modules/apps/27-interchain-accounts/controller/migrations/v6/migrations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,7 @@ func (suite *MigrationsTestSuite) SetupPath() error {
return err
}

if err := suite.path.EndpointB.ChanOpenConfirm(); err != nil {
return err
}

return nil
return suite.path.EndpointB.ChanOpenConfirm()
}

func (suite *MigrationsTestSuite) RegisterInterchainAccount(endpoint *ibctesting.Endpoint, owner string) error {
Expand Down Expand Up @@ -101,22 +97,22 @@ func (suite *MigrationsTestSuite) TestMigrateICS27ChannelCapability() {
// note: suite.SetupPath() now claims the chanel capability using icacontroller for "channel-0"
capName := host.ChannelCapabilityPath(suite.path.EndpointA.ChannelConfig.PortID, channeltypes.FormatChannelIdentifier(1))

cap, err := suite.chainA.GetSimApp().ScopedIBCKeeper.NewCapability(suite.chainA.GetContext(), capName)
capability, err := suite.chainA.GetSimApp().ScopedIBCKeeper.NewCapability(suite.chainA.GetContext(), capName)
suite.Require().NoError(err)

err = suite.chainA.GetSimApp().ScopedICAMockKeeper.ClaimCapability(suite.chainA.GetContext(), cap, capName)
err = suite.chainA.GetSimApp().ScopedICAMockKeeper.ClaimCapability(suite.chainA.GetContext(), capability, capName)
suite.Require().NoError(err)

// assert the capability is owned by the mock module
cap, found := suite.chainA.GetSimApp().ScopedICAMockKeeper.GetCapability(suite.chainA.GetContext(), capName)
suite.Require().NotNil(cap)
capability, found := suite.chainA.GetSimApp().ScopedICAMockKeeper.GetCapability(suite.chainA.GetContext(), capName)
suite.Require().NotNil(capability)
suite.Require().True(found)

isAuthenticated := suite.chainA.GetSimApp().ScopedICAMockKeeper.AuthenticateCapability(suite.chainA.GetContext(), cap, capName)
isAuthenticated := suite.chainA.GetSimApp().ScopedICAMockKeeper.AuthenticateCapability(suite.chainA.GetContext(), capability, capName)
suite.Require().True(isAuthenticated)

cap, found = suite.chainA.GetSimApp().ScopedICAControllerKeeper.GetCapability(suite.chainA.GetContext(), capName)
suite.Require().Nil(cap)
capability, found = suite.chainA.GetSimApp().ScopedICAControllerKeeper.GetCapability(suite.chainA.GetContext(), capName)
suite.Require().Nil(capability)
suite.Require().False(found)

suite.ResetMemStore() // empty the x/capability in-memory store
Expand All @@ -132,24 +128,24 @@ func (suite *MigrationsTestSuite) TestMigrateICS27ChannelCapability() {
suite.Require().NoError(err)

// assert the capability is now owned by the ICS27 controller submodule
cap, found = suite.chainA.GetSimApp().ScopedICAControllerKeeper.GetCapability(suite.chainA.GetContext(), capName)
suite.Require().NotNil(cap)
capability, found = suite.chainA.GetSimApp().ScopedICAControllerKeeper.GetCapability(suite.chainA.GetContext(), capName)
suite.Require().NotNil(capability)
suite.Require().True(found)

isAuthenticated = suite.chainA.GetSimApp().ScopedICAControllerKeeper.AuthenticateCapability(suite.chainA.GetContext(), cap, capName)
isAuthenticated = suite.chainA.GetSimApp().ScopedICAControllerKeeper.AuthenticateCapability(suite.chainA.GetContext(), capability, capName)
suite.Require().True(isAuthenticated)

cap, found = suite.chainA.GetSimApp().ScopedICAMockKeeper.GetCapability(suite.chainA.GetContext(), capName)
suite.Require().Nil(cap)
capability, found = suite.chainA.GetSimApp().ScopedICAMockKeeper.GetCapability(suite.chainA.GetContext(), capName)
suite.Require().Nil(capability)
suite.Require().False(found)

// ensure channel capability for "channel-0" is still owned by the controller
capName = host.ChannelCapabilityPath(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)
cap, found = suite.chainA.GetSimApp().ScopedICAControllerKeeper.GetCapability(suite.chainA.GetContext(), capName)
suite.Require().NotNil(cap)
capability, found = suite.chainA.GetSimApp().ScopedICAControllerKeeper.GetCapability(suite.chainA.GetContext(), capName)
suite.Require().NotNil(capability)
suite.Require().True(found)

isAuthenticated = suite.chainA.GetSimApp().ScopedICAControllerKeeper.AuthenticateCapability(suite.chainA.GetContext(), cap, capName)
isAuthenticated = suite.chainA.GetSimApp().ScopedICAControllerKeeper.AuthenticateCapability(suite.chainA.GetContext(), capability, capName)
suite.Require().True(isAuthenticated)

suite.AssertMockCapabiltiesUnchanged()
Expand All @@ -159,29 +155,29 @@ func (suite *MigrationsTestSuite) TestMigrateICS27ChannelCapability() {
// 1. A capability with a single owner
// 2. A capability with two owners, neither of which is "ibc"
func (suite *MigrationsTestSuite) CreateMockCapabilities() {
cap, err := suite.chainA.GetSimApp().ScopedIBCMockKeeper.NewCapability(suite.chainA.GetContext(), "mock_one")
capability, err := suite.chainA.GetSimApp().ScopedIBCMockKeeper.NewCapability(suite.chainA.GetContext(), "mock_one")
suite.Require().NoError(err)
suite.Require().NotNil(cap)
suite.Require().NotNil(capability)

cap, err = suite.chainA.GetSimApp().ScopedICAMockKeeper.NewCapability(suite.chainA.GetContext(), "mock_two")
capability, err = suite.chainA.GetSimApp().ScopedICAMockKeeper.NewCapability(suite.chainA.GetContext(), "mock_two")
suite.Require().NoError(err)
suite.Require().NotNil(cap)
suite.Require().NotNil(capability)

err = suite.chainA.GetSimApp().ScopedIBCMockKeeper.ClaimCapability(suite.chainA.GetContext(), cap, "mock_two")
err = suite.chainA.GetSimApp().ScopedIBCMockKeeper.ClaimCapability(suite.chainA.GetContext(), capability, "mock_two")
suite.Require().NoError(err)
}

// AssertMockCapabiltiesUnchanged authenticates the mock capabilities created at the start of the test to ensure they remain unchanged
func (suite *MigrationsTestSuite) AssertMockCapabiltiesUnchanged() {
cap, found := suite.chainA.GetSimApp().ScopedIBCMockKeeper.GetCapability(suite.chainA.GetContext(), "mock_one")
capability, found := suite.chainA.GetSimApp().ScopedIBCMockKeeper.GetCapability(suite.chainA.GetContext(), "mock_one")
suite.Require().True(found)
suite.Require().NotNil(cap)
suite.Require().NotNil(capability)

cap, found = suite.chainA.GetSimApp().ScopedIBCMockKeeper.GetCapability(suite.chainA.GetContext(), "mock_two")
capability, found = suite.chainA.GetSimApp().ScopedIBCMockKeeper.GetCapability(suite.chainA.GetContext(), "mock_two")
suite.Require().True(found)
suite.Require().NotNil(cap)
suite.Require().NotNil(capability)

isAuthenticated := suite.chainA.GetSimApp().ScopedICAMockKeeper.AuthenticateCapability(suite.chainA.GetContext(), cap, "mock_two")
isAuthenticated := suite.chainA.GetSimApp().ScopedICAMockKeeper.AuthenticateCapability(suite.chainA.GetContext(), capability, "mock_two")
suite.Require().True(isAuthenticated)
}

Expand Down
6 changes: 1 addition & 5 deletions modules/apps/27-interchain-accounts/controller/types/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,7 @@ func DefaultParams() Params {

// Validate validates all controller submodule parameters
func (p Params) Validate() error {
if err := validateEnabledType(p.ControllerEnabled); err != nil {
return err
}

return nil
return validateEnabledType(p.ControllerEnabled)
}

// ParamSetPairs implements params.ParamSet
Expand Down
18 changes: 3 additions & 15 deletions modules/apps/27-interchain-accounts/genesis/types/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,7 @@ func (gs GenesisState) Validate() error {
return err
}

if err := gs.HostGenesisState.Validate(); err != nil {
return err
}

return nil
return gs.HostGenesisState.Validate()
}

// DefaultControllerGenesis creates and returns the default interchain accounts ControllerGenesisState
Expand Down Expand Up @@ -81,11 +77,7 @@ func (gs ControllerGenesisState) Validate() error {
}
}

if err := gs.Params.Validate(); err != nil {
return err
}

return nil
return gs.Params.Validate()
}

// DefaultHostGenesis creates and returns the default interchain accounts HostGenesisState
Expand Down Expand Up @@ -132,9 +124,5 @@ func (gs HostGenesisState) Validate() error {
return err
}

if err := gs.Params.Validate(); err != nil {
return err
}

return nil
return gs.Params.Validate()
}
6 changes: 1 addition & 5 deletions modules/apps/27-interchain-accounts/host/ibc_module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,11 +106,7 @@ func SetupICAPath(path *ibctesting.Path, owner string) error {
return err
}

if err := path.EndpointB.ChanOpenConfirm(); err != nil {
return err
}

return nil
return path.EndpointB.ChanOpenConfirm()
}

// Test initiating a ChanOpenInit using the host chain instead of the controller chain
Expand Down
4 changes: 2 additions & 2 deletions modules/apps/27-interchain-accounts/host/keeper/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ import (
// InitGenesis initializes the interchain accounts host application state from a provided genesis state
func InitGenesis(ctx sdk.Context, keeper Keeper, state genesistypes.HostGenesisState) {
if !keeper.HasCapability(ctx, state.Port) {
cap := keeper.BindPort(ctx, state.Port)
if err := keeper.ClaimCapability(ctx, cap, host.PortPath(state.Port)); err != nil {
capability := keeper.BindPort(ctx, state.Port)
if err := keeper.ClaimCapability(ctx, capability, host.PortPath(state.Port)); err != nil {
panic(fmt.Sprintf("could not claim port capability: %v", err))
}
}
Expand Down
6 changes: 1 addition & 5 deletions modules/apps/27-interchain-accounts/host/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,7 @@ func SetupICAPath(path *ibctesting.Path, owner string) error {
return err
}

if err := path.EndpointB.ChanOpenConfirm(); err != nil {
return err
}

return nil
return path.EndpointB.ChanOpenConfirm()
}

// RegisterInterchainAccount is a helper function for starting the channel handshake
Expand Down
6 changes: 1 addition & 5 deletions modules/apps/27-interchain-accounts/host/types/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,7 @@ func (p Params) Validate() error {
return err
}

if err := validateAllowlist(p.AllowMessages); err != nil {
return err
}

return nil
return validateAllowlist(p.AllowMessages)
}

// ParamSetPairs implements params.ParamSet
Expand Down
4 changes: 2 additions & 2 deletions modules/apps/27-interchain-accounts/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,8 @@ func (am AppModule) InitModule(ctx sdk.Context, controllerParams controllertypes
if am.hostKeeper != nil {
am.hostKeeper.SetParams(ctx, hostParams)

cap := am.hostKeeper.BindPort(ctx, types.HostPortID)
if err := am.hostKeeper.ClaimCapability(ctx, cap, ibchost.PortPath(types.HostPortID)); err != nil {
capability := am.hostKeeper.BindPort(ctx, types.HostPortID)
if err := am.hostKeeper.ClaimCapability(ctx, capability, ibchost.PortPath(types.HostPortID)); err != nil {
panic(fmt.Sprintf("could not claim port capability: %v", err))
}
}
Expand Down
12 changes: 2 additions & 10 deletions modules/apps/29-fee/ibc_middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,11 +180,7 @@ func (im IBCMiddleware) OnChanCloseInit(
return types.ErrFeeModuleLocked
}

if err := im.keeper.RefundFeesOnChannelClosure(ctx, portID, channelID); err != nil {
return err
}

return nil
return im.keeper.RefundFeesOnChannelClosure(ctx, portID, channelID)
}

// OnChanCloseConfirm implements the IBCMiddleware interface
Expand All @@ -205,11 +201,7 @@ func (im IBCMiddleware) OnChanCloseConfirm(
return types.ErrFeeModuleLocked
}

if err := im.keeper.RefundFeesOnChannelClosure(ctx, portID, channelID); err != nil {
return err
}

return nil
return im.keeper.RefundFeesOnChannelClosure(ctx, portID, channelID)
}

// OnRecvPacket implements the IBCMiddleware interface.
Expand Down
6 changes: 1 addition & 5 deletions modules/apps/29-fee/ica_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,7 @@ func SetupPath(path *ibctesting.Path, owner string) error {
return err
}

if err := path.EndpointB.ChanOpenConfirm(); err != nil {
return err
}

return nil
return path.EndpointB.ChanOpenConfirm()
}

// RegisterInterchainAccount invokes the the InterchainAccounts entrypoint, routes a new MsgChannelOpenInit to the appropriate handler,
Expand Down
6 changes: 1 addition & 5 deletions modules/apps/29-fee/types/fee.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,7 @@ func (p PacketFee) Validate() error {
return ErrRelayersNotEmpty
}

if err := p.Fee.Validate(); err != nil {
return err
}

return nil
return p.Fee.Validate()
}

// NewPacketFees creates and returns a new PacketFees struct including a list of type PacketFee
Expand Down
12 changes: 2 additions & 10 deletions modules/apps/29-fee/types/msgs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,7 @@ func (msg MsgPayPacketFee) ValidateBasic() error {
return ErrRelayersNotEmpty
}

if err := msg.Fee.Validate(); err != nil {
return err
}

return nil
return msg.Fee.Validate()
}

// GetSigners implements sdk.Msg
Expand Down Expand Up @@ -173,11 +169,7 @@ func (msg MsgPayPacketFeeAsync) ValidateBasic() error {
return err
}

if err := msg.PacketFee.Validate(); err != nil {
return err
}

return nil
return msg.PacketFee.Validate()
}

// GetSigners implements sdk.Msg
Expand Down
4 changes: 2 additions & 2 deletions modules/apps/transfer/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ func (k Keeper) HasCapability(ctx sdk.Context, portID string) bool {
// BindPort defines a wrapper function for the ort Keeper's function in
// order to expose it to module's InitGenesis function
func (k Keeper) BindPort(ctx sdk.Context, portID string) error {
cap := k.portKeeper.BindPort(ctx, portID)
return k.ClaimCapability(ctx, cap, host.PortPath(portID))
capability := k.portKeeper.BindPort(ctx, portID)
return k.ClaimCapability(ctx, capability, host.PortPath(portID))
}

// GetPort returns the portID for the transfer module. Used in ExportGenesis
Expand Down
Loading