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

WIP: Paramstore Refactor Submodules #2233

Closed
wants to merge 12 commits into from
Closed

WIP: Paramstore Refactor Submodules #2233

wants to merge 12 commits into from

Conversation

mossid
Copy link
Contributor

@mossid mossid commented Sep 4, 2018 •
edited
Loading

Split from #1772, Contains gas parameter, consensus parameter, circuit breaker submodules; consensus parameter in baseapp.

Closes: #1007
Addresses: #1008

Dependent on: #2232

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added entries in PENDING.md with issue #

  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@mossid mossid mentioned this pull request Sep 4, 2018
Closed
8 tasks
@ValarDragon
Copy link
Contributor

The diff here is already quite massive. Can you move alot of the refactoring things, e.g.

func clone(bz []byte) (res []byte) {
	res = make([]byte, len(bz))
	copy(res, bz)
	return
}
func (s prefixStore) key(key []byte) (res []byte) {
	res = clone(s.prefix)
	res = append(res, key...)
	return
}

to a separate PR? (You can base this PR off that one)

ValarDragon
ValarDragon requested changes Sep 4, 2018
View reviewed changes
store/prefixstore.go
return
}

func (s prefixStore) key(key []byte) (res []byte) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move to a separate refactoring PR

x/params/circuit/ante.go
@@ -0,0 +1,22 @@
package circuit
Copy link
Contributor

Choose a reason for hiding this comment

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

If feasible, can we move the inclusion of circuit breaker stuff to a separate PR. This PR is already super big, and circuit breaker seems like it should be reviewed independently of the other changes that are going on.

x/params/space.go

// nolint - reexport
type Space = space.Space
type ReadOnlySpace = space.ReadOnlySpace
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add comments for what these are for

x/params/space/test_common.go
)

// Returns components for testing
func DefaultTestComponents(t *testing.T) (sdk.Context, Space, func() sdk.CommitID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you intend for this to be used in other modules? If not please make these unexported.

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 is used by the submodules, and also can be used for modules those uses paramstore internally.

ValarDragon
ValarDragon reviewed Sep 4, 2018
View reviewed changes
x/params/gas/ante.go
@@ -0,0 +1,40 @@
package gas
Copy link
Contributor

Choose a reason for hiding this comment

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

If feasible, can we move making gas use the param store into a separate PR? This PR is already quite big, and this could likeley be reviewed independently.

@mossid
Copy link
Contributor Author

mossid commented Sep 4, 2018

I think the reason that it looks big is that this PR is based on (and containing) #2232. This PR only contains (submodules) and consensus parameter related codes. If you are okay with it, I can set the base branch of this PR to #2232 for review so we can see diffs more easily. @ValarDragon

@ValarDragon
Copy link
Contributor

That sounds good to me!

@mossid mossid changed the base branch from develop to joon/paramstore-refactor-base September 4, 2018 17:36
@mossid mossid added the wip label Sep 4, 2018
@mossid mossid force-pushed the joon/paramstore-refactor-base branch 2 times, most recently from 8de065f to 1285a4a Compare September 11, 2018 07:20
@rigelrozanski
Copy link
Contributor

What's the status on these PR's

@mossid mossid force-pushed the joon/paramstore-refactor-base branch from 1285a4a to 9850ab8 Compare September 13, 2018 11:17
@mossid mossid force-pushed the joon/paramstore-refactor-submodules branch from 6e0394a to cef7fb4 Compare September 13, 2018 13:48
@codecov
Copy link

codecov bot commented Sep 13, 2018

Codecov Report

Merging #2233 into joon/paramstore-refactor-base will increase coverage by 2.93%.
The diff coverage is 55.88%.

@@                        Coverage Diff                        @@
##           joon/paramstore-refactor-base    #2233      +/-   ##
=================================================================
+ Coverage                          60.76%   63.69%   +2.93%     
=================================================================
  Files                                134      148      +14     
  Lines                               8191     8741     +550     
=================================================================
+ Hits                                4977     5568     +591     
+ Misses                              2897     2802      -95     
- Partials                             317      371      +54

@mossid mossid force-pushed the joon/paramstore-refactor-base branch 2 times, most recently from 5ff5ff3 to 0660f62 Compare September 20, 2018 18:13
@mossid mossid force-pushed the joon/paramstore-refactor-base branch from c73bd3d to 164ffb6 Compare September 26, 2018 19:07
@mossid mossid force-pushed the joon/paramstore-refactor-base branch from 7cf91b6 to 9fc7e80 Compare October 6, 2018 16:16
@cwgoes
Copy link
Contributor

cwgoes commented Oct 18, 2018

Closing, but leaving the branch for future work.

@cwgoes cwgoes closed this Oct 18, 2018
@mircea-c mircea-c deleted the joon/paramstore-refactor-submodules branch April 26, 2019 15:53
chillyvee pushed a commit to chillyvee/cosmos-sdk that referenced this pull request Mar 1, 2024
@dependabot
chore(deps): bump golang.org/x/net from 0.5.0 to 0.7.0 (cosmos#2233)
e823c4e 
Bumps [golang.org/x/net](https://github.com/golang/net) from 0.5.0 to 0.7.0.
- [Release notes](https://github.com/golang/net/releases)
- [Commits](golang/net@v0.5.0...v0.7.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ValarDragon ValarDragon ValarDragon requested changes

@cwgoes cwgoes Awaiting requested review from cwgoes

@ebuchman ebuchman Awaiting requested review from ebuchman

@rigelrozanski rigelrozanski Awaiting requested review from rigelrozanski

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

Successfully merging this pull request may close these issues.
4 participants
@mossid @ValarDragon @rigelrozanski @cwgoes