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

Staking store keys bug fix + memory improvement #2435

Merged
merged 2 commits into from
Oct 4, 2018
Merged

Conversation

ValarDragon
Copy link
Contributor

Improves memory efficiency of getting store keys

This is done by removing repeated appends, which will create a new
slice if theres insufficient capacity, and instead creating a key
of the correct size, and then copying the data into it.

This PR also Removes empty bytes from ValidatorPowerRank store key

Closes #2433

Please do not squash, and review commit by commit.

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/) - n/a?
  • 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)

This is done by removing repeated appends, which will create a new
slice if theres insufficient capacity, and instead creating a key
of the correct size, and then copying the data into it.
@ValarDragon ValarDragon added ready-for-review T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). labels Oct 3, 2018
@codecov
Copy link

codecov bot commented Oct 3, 2018

Codecov Report

❗ No coverage uploaded for pull request base (develop@f8cc0af). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff             @@
##             develop    #2435   +/-   ##
==========================================
  Coverage           ?   61.18%           
==========================================
  Files              ?      123           
  Lines              ?     7396           
  Branches           ?        0           
==========================================
  Hits               ?     4525           
  Misses             ?     2558           
  Partials           ?      313

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

utACK

var (
pk1 = ed25519.GenPrivKey().PubKey()
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have unit tests for GetREDByValSrcIndexKey and GetREDByValDstIndexKey? Otherwise, this LGTM. Thanks 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have any to my knowledge. I can make some

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do that, but it can be in a separate PR.

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.

utACK

var (
pk1 = ed25519.GenPrivKey().PubKey()
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do that, but it can be in a separate PR.

@cwgoes cwgoes merged commit b4e8fe5 into develop Oct 4, 2018
@cwgoes cwgoes deleted the dev/staking_keys branch October 4, 2018 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants