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.

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

Global Paramstore #1265

Merged
merged 1 commit into from
Jul 14, 2018
Merged

Global Paramstore #1265

merged 1 commit into from
Jul 14, 2018

Conversation

mossid
Copy link
Contributor

@mossid mossid commented Jun 15, 2018

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG.md
  • Updated Gaia/Examples
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

Closes: #1159
Closes: #1161

@mossid mossid requested a review from ebuchman as a code owner June 15, 2018 05:58
@mossid mossid requested review from sunnya97 and removed request for ebuchman June 15, 2018 05:58
@mossid mossid changed the title Global Paramstore WIP: Global Paramstore Jun 15, 2018
@ebuchman ebuchman added the wip label Jun 15, 2018
@cwgoes
Copy link
Contributor

cwgoes commented Jun 15, 2018

Hmm, I wonder if it might be better to have a few explicit types instead of using reflection.

@gamarin2
Copy link
Contributor

gamarin2 commented Jun 15, 2018

Just linking these two issues that are relevant to this PR @mossid @sunnya97 :

@codecov
Copy link

codecov bot commented Jun 16, 2018

Codecov Report

Merging #1265 into master will increase coverage by 1.96%.
The diff coverage is 81.53%.

@@            Coverage Diff             @@
##           master    #1265      +/-   ##
==========================================
+ Coverage   63.78%   65.75%   +1.96%     
==========================================
  Files         122      120       -2     
  Lines        6763     6924     +161     
==========================================
+ Hits         4314     4553     +239     
+ Misses       2204     2117      -87     
- Partials      245      254       +9

@sunnya97
Copy link
Member

@cwgoes What do you mean by a few explicit types?

@mossid
Copy link
Contributor Author

mossid commented Jun 21, 2018

Suspended until #1119 is merged

@cwgoes cwgoes added the S:blocked Status: Blocked label Jun 21, 2018
@mossid mossid changed the title WIP: Global Paramstore Global Paramstore Jun 22, 2018
@mossid mossid added ready-for-review and removed S:blocked Status: Blocked wip labels Jun 22, 2018
@mossid
Copy link
Contributor Author

mossid commented Jun 22, 2018

Revert all changes those depends on #1119, ready for review

@mossid
Copy link
Contributor Author

mossid commented Jun 27, 2018

#1119 is merged. Maybe we can merge it after applying it to stake module.

@cwgoes cwgoes changed the base branch from develop to unstable July 3, 2018 21:12
mossid added a commit that referenced this pull request Jul 4, 2018
in progress

in progress

stake and slashing now params

fix gaia

fix gaia again

add msg type deactivation

delete local error

in progress

revert actual application in baseapp/gaia/stake

add test, fix apps

fix MinSignedPerWindow, pass lint

fix gaia

fix keeper_test

fit with multiple msgs

fix

apply requests

pass lint

really the last fix

fix dependency

fix keeper_test

fix lint
@cwgoes
Copy link
Contributor

cwgoes commented Jul 4, 2018

@mossid Did you want to update this with the stake module parameters prior to merge?

@cwgoes cwgoes changed the base branch from unstable to develop July 4, 2018 19:25
@mossid
Copy link
Contributor Author

mossid commented Jul 4, 2018

@cwgoes I think we can merge it for now, will make another PR for stake params.

// TODO Governance parameter?
MinSignedPerWindow = SignedBlocksWindow / 2
// TODO Temporarily set to 100 blocks for testnets
defaultSignedBlocksWindow int64 = 100
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be 40000

// TODO Temporarily set to five minutes for testnets
DowntimeUnbondDuration int64 = 60 * 5
// TODO Temporarily set to 10 minutes for testnets
defaultDowntimeUnbondDuration int64 = 60 * 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be 5 minutes

cwgoes
cwgoes previously approved these changes Jul 5, 2018
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Tested ACK, would like @rigelrozanski's opinion before merge.

@ebuchman ebuchman changed the base branch from develop to master July 11, 2018 00:28
in progress

in progress

stake and slashing now params

fix gaia

fix gaia again

add msg type deactivation

delete local error

in progress

revert actual application in baseapp/gaia/stake

add test, fix apps

fix MinSignedPerWindow, pass lint

fix gaia

fix keeper_test

fit with multiple msgs

fix

apply requests

pass lint

really the last fix

fix dependency

fix keeper_test

fix lint
@cwgoes cwgoes merged commit bdccbef into master Jul 14, 2018
@cwgoes cwgoes deleted the joon/global-paramstore branch July 14, 2018 00:12
@cwgoes cwgoes restored the joon/global-paramstore branch July 16, 2018 20:40
@cwgoes
Copy link
Contributor

cwgoes commented Jul 16, 2018

Needs to be reopened against develop I think.

@ebuchman
Copy link
Member

Needs to be reopened against develop I think.

Master was merged back to develop.

@cwgoes cwgoes deleted the joon/global-paramstore branch July 18, 2018 18:31
@abelliumnt
Copy link

@mossid
How about add the two methods in getter: GetRawIterator and GetRawReverseIterator?

func (k Getter) GetRawIterator(ctx sdk.Context, start, end string) (iterator dbm.Iterator, err error) {
	store := ctx.KVStore(k.k.key)
	iterator = store.Iterator([]byte(start),[]byte(end))
	return
}

func (k Getter) GetRawReverseIterator(ctx sdk.Context, start, end string) (iterator dbm.Iterator, err error) {
	store := ctx.KVStore(k.k.key)
	iterator = store.ReverseIterator([]byte(start),[]byte(end))
	return
}

I think they are much helpful in query a set of keys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants