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

Turn BlockChain.GetStates() into GetState() #563

Conversation

qtuu
Copy link
Contributor

@qtuu qtuu commented Oct 2, 2019

An attempt to solve Issue #510
It feels super wonky to still use AddressStateMap as a returnvalue, because it will always have just the one entry. I tried to make it return the (object) value instead, but failed misserably on a serialization-issue, breaking most of the use-cases.

If you guys want me to pursuit a better return value, i could try again...

Anyways, looking forward to your feedback :)

@longfin
Copy link
Member

longfin commented Oct 4, 2019

@qtuu Great work!

As you say, AddressStateMap has some serialization issues and we're fixing them. so I believe it's okay to ignore in this PR.

limebell
limebell previously approved these changes Oct 4, 2019
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 7, 2019

Codecov Report

Merging #563 into master will decrease coverage by 1.76%.
The diff coverage is 94.84%.

@@            Coverage Diff             @@
##           master     #563      +/-   ##
==========================================
- Coverage   90.86%   89.09%   -1.77%     
==========================================
  Files         201      177      -24     
  Lines       15230    14532     -698     
==========================================
- Hits        13838    12948     -890     
- Misses       1101     1300     +199     
+ Partials      291      284       -7
Impacted Files Coverage Δ
Libplanet.Tests/Net/SwarmTest.cs 98.94% <100%> (ø) ⬆️
Libplanet/Blockchain/BlockChain.cs 94.12% <91.11%> (-0.14%) ⬇️
Libplanet.Tests/Blockchain/BlockChainTest.cs 98.68% <97.56%> (-0.01%) ⬇️
Libplanet.Stun/Stun/Attributes/ErrorCode.cs 0% <0%> (-100%) ⬇️
Libplanet.Stun/Stun/Attributes/Nonce.cs 0% <0%> (-100%) ⬇️
Libplanet.Stun/Stun/Attributes/Realm.cs 0% <0%> (-100%) ⬇️
Libplanet.Stun/Stun/Attributes/MessageIntegrity.cs 0% <0%> (-100%) ⬇️
Libplanet.Stun/Stun/Messages/RefreshRequest.cs 0% <0%> (-100%) ⬇️
Libplanet.Stun/Stun/Attributes/Software.cs 0% <0%> (-100%) ⬇️
...planet.Stun/Stun/Messages/AllocateErrorResponse.cs 0% <0%> (-66.67%) ⬇️
... and 31 more

@dahlia
Copy link
Contributor

dahlia commented Oct 7, 2019

/rebase

@qtuu
Copy link
Contributor Author

qtuu commented Oct 7, 2019

/rebase

I am sorry. Should I have done a rebase merge? I am at loss here

qtuu and others added 6 commits October 8, 2019 00:53
When no stateReference is found, we need to return early. This was done implicitly by iterating over an empty stateReferences-HashSet.
Co-Authored-By: Seunghun Lee <waydi1@gmail.com>
Co-Authored-By: Seunghun Lee <waydi1@gmail.com>
use pattern matching

Co-Authored-By: Swen Mun <longfinfunnel@gmail.com>
@earlbread earlbread force-pushed the Turn-BlockChain-T-.GetStates()-into-GetState() branch from 2820872 to 45f82c8 Compare October 7, 2019 15:58
@earlbread
Copy link
Contributor

/rebase

I am sorry. Should I have done a rebase merge? I am at loss here

We tried to do an automatic rebase, but it didn't work. So I rebased it manually.

@longfin longfin requested a review from limebell October 7, 2019 22:24
@longfin longfin merged commit e7ccac6 into planetarium:master Oct 8, 2019
@longfin
Copy link
Member

longfin commented Oct 8, 2019

@qtuu Thanks for the contribution.

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.

5 participants