Skip to content

Update for cosmos-sdk v0.46.1 compatibility#536

Merged
tzdybal merged 4 commits intomainfrom
tzdybal/cosmos_0.46.x
Sep 27, 2022
Merged

Update for cosmos-sdk v0.46.1 compatibility#536
tzdybal merged 4 commits intomainfrom
tzdybal/cosmos_0.46.x

Conversation

@tzdybal
Copy link
Copy Markdown
Contributor

@tzdybal tzdybal commented Sep 24, 2022

Resolves #529.

@tzdybal tzdybal self-assigned this Sep 24, 2022
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 24, 2022

Codecov Report

Merging #536 (eb7dbee) into main (2e000cf) will decrease coverage by 0.05%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##             main     #536      +/-   ##
==========================================
- Coverage   55.81%   55.76%   -0.06%     
==========================================
  Files          50       50              
  Lines        9555     9544      -11     
==========================================
- Hits         5333     5322      -11     
  Misses       3428     3428              
  Partials      794      794              
Impacted Files Coverage Δ
state/executor.go 70.79% <ø> (ø)
rpc/client/client.go 45.45% <80.00%> (-0.50%) ⬇️
node/node.go 62.26% <100.00%> (+0.51%) ⬆️
mempool/v1/mempool.go 86.09% <0.00%> (-0.63%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@tzdybal
Copy link
Copy Markdown
Contributor Author

tzdybal commented Sep 25, 2022

docker-build will not work until optimint-enabled-cosmos-sdk is updated.

@tzdybal tzdybal marked this pull request as ready for review September 25, 2022 08:51
@tzdybal tzdybal requested a review from jbowen93 September 26, 2022 10:59
jbowen93
jbowen93 previously approved these changes Sep 26, 2022
Copy link
Copy Markdown
Contributor

@jbowen93 jbowen93 left a comment

Choose a reason for hiding this comment

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

utACK. LGTM

Comment thread node/node.go Outdated
"github.com/libp2p/go-libp2p/core/crypto"
"go.uber.org/multierr"

abcicli "github.com/tendermint/tendermint/abci/client"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: I don't like the abbreviation abcicli as it makes me think it's related to a command line interface (CLI). I'd prefer either abciclient

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If following strict Go style we could also use ac

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For some reason I thought it's the way it is used in cosmos-sdk... but it's not, so I fixed to abciclient.

Copy link
Copy Markdown
Contributor

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Left to minor nits. LGTM

@@ -1,11 +1,11 @@
FROM --platform=$BUILDPLATFORM golang:1.17-alpine AS build-env
FROM --platform=$BUILDPLATFORM golang:1.19-alpine AS build-env
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't we consistently use the same go version here as in go.mod?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

go directive from go.mod is defined as:

The minimum version of Go required to compile packages in this module.

I'm not sure what is the actual minimal version TBH (will check this out). Code-wise I don't think there are any cases prohibiting older Go versions.

I used latest version for CI, just because it's latest available.

Comment thread node/node.go Outdated
return nil, fmt.Errorf("error starting proxy app connections: %w", err)
}

func NewNode(ctx context.Context, conf config.NodeConfig, p2pKey crypto.PrivKey, signingKey crypto.PrivKey, appClient abcicli.Client, genesis *tmtypes.GenesisDoc, logger log.Logger) (*Node, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: I would recommend line-breaks per param here.

@tzdybal tzdybal force-pushed the tzdybal/cosmos_0.46.x branch from 57a8fda to eb7dbee Compare September 27, 2022 10:59
@tzdybal tzdybal force-pushed the tzdybal/cosmos_0.46.x branch from eb7dbee to 5e17592 Compare September 27, 2022 13:33
@tzdybal tzdybal merged commit 722b44e into main Sep 27, 2022
@tzdybal tzdybal deleted the tzdybal/cosmos_0.46.x branch September 27, 2022 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update Optimint for v0.46.x compatibility

4 participants