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

Jump to bottom

feat: 45 annotation #2039

Closed
wants to merge 4 commits into from
Closed

feat: 45 annotation #2039

wants to merge 4 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Jan 30, 2022 •
edited
Loading

Addresses:

#2030
#1998

Works towards:

#2038
#2156

Changes:

  • Adds annotation to app.go
  • upgrades to cosmos-sdk v0.45.0 v0.45.1
  • puts all modules in BeginBlockers, EndBlockers, and SetOrderInitGenesis
  • upgrades starport itself to Go 1.17
  • upgrades apps scaffolded with starport to Go 1.17
  • Enables rocksdb support for apps scaffolded with starport
  • Changes the version of Go used in CI to 1.17

@faddat faddat changed the title 45 annotation feat: 45 annotation Jan 30, 2022
@faddat faddat marked this pull request as ready for review January 30, 2022 09:11
@faddat faddat marked this pull request as draft January 30, 2022 09:14
@faddat
Copy link
Contributor Author

faddat commented Jan 30, 2022

I have a feeling that we should not obey the linter here:

https://github.com/tendermint/starport/runs/4996045953?check_suite_focus=true#step:5:80

wdyt?

@faddat
Copy link
Contributor Author

faddat commented Jan 30, 2022

In the end I decided to obey the linter....

@faddat faddat mentioned this pull request Jan 30, 2022
Closed
@faddat faddat marked this pull request as ready for review January 30, 2022 10:24
@faddat faddat mentioned this pull request Jan 30, 2022
Closed
@Ruborcalor
Copy link

Hello! Cosmos-sdk v0.45.0 introduces the following state breaking change:

  • "We now charge gas in two new places: on .Seek() even if there are no entries, and for the key length (on top of the value length)."

How do you expect this to effect gas consumption? Could it possibly double in the worst case?

@faddat
Copy link
Contributor Author

faddat commented Feb 11, 2022

@Ruborcalor not really sure what you mean here

@faddat faddat mentioned this pull request Mar 22, 2022
Closed
@faddat faddat marked this pull request as draft March 22, 2022 22:59
alessio
alessio suggested changes Mar 25, 2022
View reviewed changes
Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

Thank you for taking the time to open this pull request and helping to make Starport better. I left some comments.

go.mod
@@ -1,6 +1,6 @@
module github.com/tendermint/starport

go 1.16
go 1.17
Copy link
Contributor

Choose a reason for hiding this comment

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

Source code and dependencies do not require this - if they did, go mod would bump this automatically.
Please revert.

starport/pkg/cosmosanalysis/module/testdata/planet/go.mod
@@ -1,4 +1,4 @@
module github.com/tendermint/planet

go 1.16
go 1.17
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto, see above.

starport/templates/app/stargate/app/app.go.plush
@@ -19,74 +21,132 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/version"

// CLI tooling
"github.com/spf13/cast"
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this introduced manually? The indentation looks funky.

starport/templates/app/stargate/go.mod.plush
@@ -1,27 +1,132 @@
module <%= ModulePath %>

go 1.16
go 1.17
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto, see above.

starport/templates/app/stargate/go.sum
@@ -1,5 +1,6 @@
4d63.com/gochecknoglobals v0.0.0-20201008074935-acfc0b28355a/go.mod h1:wfdC5ZjKSPr7CybKEcgJhUOgeAQW1+7WcyK8OvUilfo=
bazil.org/fuse v0.0.0-20160811212531-371fbbdaa898/go.mod h1:Xbm+BRKSBEpa4q4hTSxohYNQpsxXPbPry4JJWOB3LB8=
bazil.org/fuse v0.0.0-20200407214033-5883e5a4b512/go.mod h1:FbcW6z/2VytnFDhZfumh8Ss8zxHE6qpMP5sHTRe0EaM=
Copy link
Contributor

Choose a reason for hiding this comment

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

Please run go mod tidy to purge deps that are no longer needed.

starport/templates/module/create/ibc.go
@@ -7,7 +7,6 @@ import (
"github.com/gobuffalo/genny"
"github.com/gobuffalo/plush"
"github.com/gobuffalo/plushgen"

Copy link
Contributor

Choose a reason for hiding this comment

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

Several separators (i.e. blank lines) were removed, why? What's the justification? I'd advise reverting unnecessary changes in order to reduce the diff's size.

starport/templates/typed/typed.go
@@ -44,5 +43,9 @@ func Box(box packd.Walker, opts *Options, g *genny.Generator) error {
g.Transformer(genny.Replace("{{typeName}}", opts.TypeName.Snake))

// Create the 'testutil' package with the test helpers
return testutil.Register(g, opts.AppPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why? This is a more concise (and readable form). Please revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am submitting a new PR, which contains only changes to the template.

@faddat
Copy link
Contributor Author

faddat commented Mar 30, 2022

closing this, and opening again, with a smaller-scope version.

In the future, if there is a scoping issue on a PR that has been explicitly approved in issues, I'd really love to know about that before 2 months in.

@faddat faddat closed this Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alessio alessio alessio requested changes

@fadeev fadeev Awaiting requested review from fadeev

@ilgooz ilgooz Awaiting requested review from ilgooz

@ivanovpetr ivanovpetr Awaiting requested review from ivanovpetr

@lumtis lumtis Awaiting requested review from lumtis

@Pantani Pantani Awaiting requested review from Pantani

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@faddat @Ruborcalor @alessio