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 mossid force-pushed the joon/global-paramstore branch from 2a4c76a to ac1ed5a Compare June 20, 2018 04:35
@mossid mossid force-pushed the joon/global-paramstore branch from 94bb76c to b5a6695 Compare June 21, 2018 03:21
@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 force-pushed the joon/global-paramstore branch from b5a6695 to 9e322bf Compare June 21, 2018 21:29
@mossid mossid force-pushed the joon/global-paramstore branch from 9e322bf to 0967763 Compare June 21, 2018 22:07
@mossid mossid force-pushed the joon/global-paramstore branch from fc421d9 to ba13dcb Compare June 21, 2018 23:07
@mossid mossid force-pushed the joon/global-paramstore branch from ba13dcb to 1bd3318 Compare June 21, 2018 23:26
@mossid mossid force-pushed the joon/global-paramstore branch from 1bd3318 to f956c9d Compare June 22, 2018 00:27
@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 mossid force-pushed the joon/global-paramstore branch from 82f41e0 to 1f35556 Compare July 4, 2018 01:31
@mossid mossid force-pushed the joon/global-paramstore branch from 1f35556 to abedd24 Compare July 4, 2018 03:51
@mossid mossid force-pushed the joon/global-paramstore branch from abedd24 to 905ef51 Compare July 4, 2018 04:04
@mossid mossid force-pushed the joon/global-paramstore branch from 905ef51 to 340804e Compare July 4, 2018 04:09
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

@mossid mossid requested a review from rigelrozanski as a code owner July 5, 2018 00:19
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
@mossid mossid force-pushed the joon/global-paramstore branch from d962dd9 to fbf71b0 Compare July 12, 2018 05:30
@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
@HaoyangLiu
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