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

Change BlockChain to implement IReadOnlyList #205

Merged
merged 1 commit into from Apr 16, 2019

Conversation

Projects
None yet
3 participants
@earlbread
Copy link
Member

commented Apr 16, 2019

This changes BlockChain<T> to implement IReadOnlyList<Block<T>>.

@earlbread earlbread requested review from dahlia and longfin Apr 16, 2019

@earlbread earlbread self-assigned this Apr 16, 2019

@earlbread earlbread force-pushed the earlbread:blockchain-impl-irol branch from 6933968 to ad59766 Apr 16, 2019

Show resolved Hide resolved CHANGES.md Outdated
Show resolved Hide resolved Libplanet.Tests/Blockchain/Policies/BlockPolicyTest.cs Outdated
Show resolved Hide resolved Libplanet/Blockchain/BlockChain.cs Outdated
@@ -111,7 +118,7 @@ public Block<T> Tip
}

public void Validate(
IEnumerable<Block<T>> blocks,
IReadOnlyList<Block<T>> blocks,

This comment has been minimized.

Copy link
@dahlia

dahlia Apr 16, 2019

Member

As this is a public method, this change also should be written in the changelog.

@@ -43,6 +43,6 @@ DateTimeOffset currentTime
/// followed by a new <see cref="Block{T}"/> to be mined.</param>
/// <returns>A right <see cref="Block{T}.Difficulty"/>
/// for a new <see cref="Block{T}"/> to be mined.</returns>
int GetNextBlockDifficulty(IEnumerable<Block<T>> blocks);
int GetNextBlockDifficulty(IReadOnlyList<Block<T>> blocks);

This comment has been minimized.

Copy link
@dahlia

dahlia Apr 16, 2019

Member

Changes in this file also break API backward compatibility, we need to write the corresponding changelogs.

@codecov

This comment has been minimized.

Copy link

commented Apr 16, 2019

Codecov Report

Merging #205 into master will decrease coverage by 3.25%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master     #205      +/-   ##
==========================================
- Coverage   87.46%   84.21%   -3.26%     
==========================================
  Files          72       72              
  Lines        3367     3370       +3     
==========================================
- Hits         2945     2838     -107     
- Misses        422      532     +110
Impacted Files Coverage Δ
Libplanet/Blockchain/Policies/BlockPolicy.cs 100% <ø> (ø) ⬆️
Libplanet/Blockchain/BlockChain.cs 98.86% <75%> (-0.38%) ⬇️
Libplanet/Net/IceServer.cs 0% <0%> (-100%) ⬇️
Libplanet/Net/IceServerException.cs 0% <0%> (-100%) ⬇️
Libplanet/Net/NetworkStreamProxy.cs 0% <0%> (-77.78%) ⬇️
Libplanet/Net/Swarm.cs 81.73% <0%> (-6.09%) ⬇️

@earlbread earlbread force-pushed the earlbread:blockchain-impl-irol branch from ad59766 to 7de4f42 Apr 16, 2019

@earlbread

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

I applied review comments and amended the commit. Please take a look. @dahlia @longfin

CHANGES.md Outdated
@@ -29,11 +29,19 @@ To be released.
given.
- Fixed a bug that TURN relay had been disconnected when being connected for longer than 5
minutes.
- `BlockChain<T>` now implements `IReadOnlyList<Block<T>>`. [[#205]]
`BlockChain<T>.Validate()` now receives `IReadOnlyList<Block<<T>>`

This comment has been minimized.

Copy link
@dahlia

dahlia Apr 16, 2019

Member

A bullet seems missed.

Suggested change
`BlockChain<T>.Validate()` now receives `IReadOnlyList<Block<<T>>`
- `BlockChain<T>.Validate()` now receives `IReadOnlyList<Block<<T>>`
CHANGES.md Outdated
instead of `IEnumerable<Block<T>>`. [[#205]]
- `IBlockPolicy<T>.ValidateBlocks()` and
`IBlockPolicy<T>.GetNextBlockDifficulty()`
now receives `IReadOnlyList<Block<<T>>` instead of `IEnumerable<Block<T>>`.

This comment has been minimized.

Copy link
@dahlia

dahlia Apr 16, 2019

Member

Plural present tense.

Suggested change
now receives `IReadOnlyList<Block<<T>>` instead of `IEnumerable<Block<T>>`.
now receive `IReadOnlyList<Block<<T>>` instead of `IEnumerable<Block<T>>`.

As a non-native speaker, I prefer to use “became to do” instead of “now does/do”, because in the latter way a verb should inflect depending on its subject (singular or plural). On the other hand, a verb following “became to do” is always singular present, so less prone to make a grammatical mistake. 🙄

@earlbread earlbread force-pushed the earlbread:blockchain-impl-irol branch 2 times, most recently from bc3c0c4 to 5ab8e60 Apr 16, 2019

@codecov

This comment has been minimized.

Copy link

commented Apr 16, 2019

Codecov Report

Merging #205 into master will decrease coverage by 3.25%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master     #205      +/-   ##
==========================================
- Coverage   87.46%   84.21%   -3.26%     
==========================================
  Files          72       72              
  Lines        3367     3370       +3     
==========================================
- Hits         2945     2838     -107     
- Misses        422      532     +110
Impacted Files Coverage Δ
Libplanet/Blockchain/Policies/BlockPolicy.cs 100% <ø> (ø) ⬆️
Libplanet/Blockchain/BlockChain.cs 98.86% <75%> (-0.38%) ⬇️
Libplanet/Net/IceServer.cs 0% <0%> (-100%) ⬇️
Libplanet/Net/IceServerException.cs 0% <0%> (-100%) ⬇️
Libplanet/Net/NetworkStreamProxy.cs 0% <0%> (-77.78%) ⬇️
Libplanet/Net/Swarm.cs 81.73% <0%> (-6.09%) ⬇️

@earlbread earlbread force-pushed the earlbread:blockchain-impl-irol branch from 5ab8e60 to 291483e Apr 16, 2019

@earlbread

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

I fixed the changelog and amended the commit. Could you please review this again? @dahlia @longfin

@dahlia

dahlia approved these changes Apr 16, 2019

@earlbread earlbread merged commit b84b302 into planetarium:master Apr 16, 2019

3 of 5 checks passed

codecov/patch 75% of diff hit (target 87.46%)
Details
codecov/project 84.21% (-3.26%) compared to 920c7ea
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
changelog This contains self-describing changelog.
Details

@earlbread earlbread deleted the earlbread:blockchain-impl-irol branch Apr 16, 2019

internal IStore Store { get; }

/// <inheritdoc/>
public Block<T> this[int index] => this[(long)index];

This comment has been minimized.

Copy link
@dahlia

dahlia Apr 16, 2019

Member

@earlbread Although it's late, we should make it throw ArgumentOutOfRangeException instead of IndexOutOfRangeException, because collection types in System.Collections.Generic throw ArgumentOutOfRangeException and only arrays throw IndexOutOfRangeException. See also this SO answer:

Note that List<T> throws ArgumentOutOfRangeException for the same cases where arrays use IndexOutOfRangeException.

Could you change this in the next patch?

This comment has been minimized.

Copy link
@earlbread

earlbread Apr 16, 2019

Author Member

Sure, I'll change it in the next patch!

@@ -236,8 +243,9 @@ public void Append(Block<T> block, DateTimeOffset currentTime)
block.Validate(
currentTime,
a => GetStates(new[] { a }, tip).GetValueOrDefault(a));
Validate(Enumerable.Append(this, block), currentTime);
Enumerable.Append(this, block);

This comment has been minimized.

Copy link
@dahlia

dahlia Apr 19, 2019

Member

Although it's late, this seems a miss; Enumerable.Append() method returns a new IEnumerable<T> that has one more item, but this code doesn't use its return value. According to the docs:

This method does not modify the elements of the collection. Instead, it creates a copy of the collection with the new element.

To me, the code before changed seems correct. @earlbread Could you fix this in the pull request #210?

This comment has been minimized.

Copy link
@dahlia

dahlia Apr 19, 2019

Member

It seems this code was changed because the type of Validate() method's first parameter was changed from IEnumerable<Bock<T>> to IReadOnlyList<Block<T>> in this pull request. To fix this quickly, it could be like Validate(Enumerable.Append(this, block).ToList(), currentTime);, I'm skeptic if it's good though.

This comment has been minimized.

Copy link
@earlbread

earlbread Apr 19, 2019

Author Member

This seems not needed anymore. I'll remove it in #210.

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.