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

Add block hash index which has address states #232

Merged
merged 23 commits into from May 14, 2019

Conversation

Projects
None yet
3 participants
@earlbread
Copy link
Member

commented May 7, 2019

  • Added IStore.LookupStateReference<T>() method.
  • Added IStore.StoreStateReference<T>() method.
  • Added IStore.ForkStateReferences<T>() method.
  • Changed BlockChain<T>.GetStates() to use a state reference, which is a Block<T>.Hash that has the state of the Address.
  • Added documentation of BlockChain<T>.GetStates().

@earlbread earlbread requested review from dahlia and longfin May 7, 2019

@earlbread earlbread self-assigned this May 7, 2019

@earlbread earlbread force-pushed the earlbread:state-index branch 3 times, most recently from 7255b1a to 2882d91 May 8, 2019

@earlbread earlbread marked this pull request as ready for review May 9, 2019

@codecov

This comment has been minimized.

Copy link

commented May 9, 2019

Codecov Report

Merging #232 into master will increase coverage by 0.61%.
The diff coverage is 97.93%.

@@            Coverage Diff             @@
##           master     #232      +/-   ##
==========================================
+ Coverage   83.45%   84.07%   +0.61%     
==========================================
  Files          75       76       +1     
  Lines        3494     3573      +79     
==========================================
+ Hits         2916     3004      +88     
+ Misses        578      569       -9
Impacted Files Coverage Δ
Libplanet/Store/BaseStore.cs 100% <ø> (ø) ⬆️
Libplanet/Blockchain/BlockChain.cs 98.1% <100%> (+0.05%) ⬆️
Libplanet/Store/BlockSet.cs 96.55% <100%> (+5.98%) ⬆️
Libplanet/Store/NamespaceNotFoundException.cs 80% <80%> (ø)
Libplanet/Store/FileStore.cs 95.15% <98.18%> (+0.76%) ⬆️
Libplanet/Net/Swarm.cs 79.75% <0%> (+0.87%) ⬆️

@earlbread earlbread changed the title [WIP] Add block hash index which has address states Add block hash index which has address states May 9, 2019

Show resolved Hide resolved Libplanet/Store/FileStore.cs Outdated
Show resolved Hide resolved Libplanet/Store/FileStore.cs Outdated
Show resolved Hide resolved Libplanet/Store/FileStore.cs Outdated
Show resolved Hide resolved Libplanet/Store/IStore.cs Outdated
Show resolved Hide resolved Libplanet/Store/IStore.cs Outdated
Show resolved Hide resolved Libplanet/Store/IStore.cs Outdated
Show resolved Hide resolved Libplanet/Store/IStore.cs Outdated

@earlbread earlbread force-pushed the earlbread:state-index branch 3 times, most recently from e448101 to c3024c4 May 9, 2019

@earlbread

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

I applied review comments and rebased on the master. Also, I renamed methods using a word StateReference. Please take a look. @dahlia @longfin

Show resolved Hide resolved Libplanet/Store/FileStore.cs Outdated

@earlbread earlbread force-pushed the earlbread:state-index branch from 73bb8c2 to 2edc467 May 10, 2019

@earlbread

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

I amended a commit. Please take a look.

Show resolved Hide resolved Libplanet/Store/IStore.cs Outdated
Show resolved Hide resolved Libplanet/Store/IStore.cs Outdated
Show resolved Hide resolved Libplanet/Store/IStore.cs Outdated

@earlbread earlbread force-pushed the earlbread:state-index branch from eb2feb5 to e7e9705 May 10, 2019

@earlbread earlbread force-pushed the earlbread:state-index branch from e7e9705 to e19e5e9 May 10, 2019

Show resolved Hide resolved Libplanet/Store/BaseStore.cs
Show resolved Hide resolved Libplanet/Blockchain/BlockChain.cs Outdated

@earlbread earlbread force-pushed the earlbread:state-index branch from e19e5e9 to 49c4262 May 12, 2019

@earlbread

This comment has been minimized.

Copy link
Member Author

commented May 12, 2019

I rebased on the master and applied review comments. Please take a look.


for (
Block<T> block = Tip;
block.PreviousHash is HashDigest<SHA256> hash

This comment has been minimized.

Copy link
@dahlia

dahlia May 12, 2019

Member

This condition won't work if block refers to a block right after genesis, which means addresses updated by the second block (where block.Index == 1) are always excluded from the index.

Please write a test for this specific case as well.

This comment has been minimized.

Copy link
@earlbread

earlbread May 13, 2019

Author Member

I think this condition is break when the block refers to the genesis block, not the second block. But I thought it is ok because every forking must have the genesis block.
I added tests for this case and found another bug and fixed it.

@earlbread earlbread force-pushed the earlbread:state-index branch from 49c4262 to bcb5e03 May 13, 2019

@earlbread

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

I have fixed the part that uses Transaction.UpdatedAddress. Because it is for validation, it may be different from the result of BlockChain<T>.SetStates.

@earlbread earlbread force-pushed the earlbread:state-index branch from 63c835b to f814205 May 14, 2019

@longfin

This comment has been minimized.

Copy link
Member

commented May 14, 2019

I have fixed the part that uses Transaction.UpdatedAddress. Because it is for validation, it may be different from the result of BlockChain<T>.SetStates.

It would be also better if there is a regression test about it.

@longfin

This comment has been minimized.

Copy link
Member

commented May 14, 2019

I have fixed the part that uses Transaction.UpdatedAddress. Because it is for validation, it may be different from the result of BlockChain<T>.SetStates.

It would be also better if there is a regression test about it.

I've missed. it's added 👍

@dahlia

dahlia approved these changes May 14, 2019

@earlbread earlbread merged commit 2d3fd48 into planetarium:master May 14, 2019

5 checks passed

Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
changelog This contains self-describing changelog.
Details
codecov/patch 97.93% of diff hit (target 83.45%)
Details
codecov/project 84.07% (+0.61%) compared to dc9acff
Details

@earlbread earlbread deleted the earlbread:state-index branch May 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.