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

Integrate Block<T>.Validate() and .EvaluateActions() #243

Merged

Conversation

Projects
None yet
3 participants
@longfin
Copy link
Member

commented May 17, 2019

This PR integrates Block<T>.Validate() and Block<T>.EvaluateActions() into Block<T>.Evaluate() to reduce Action.Execute() during BlockChain<T>.Append().

@longfin longfin force-pushed the longfin:feature/integrate-validate-evaluate branch from 9cc7435 to 81bcc41 May 17, 2019

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

@longfin longfin marked this pull request as ready for review May 17, 2019

@codecov

This comment has been minimized.

Copy link

commented May 17, 2019

Codecov Report

Merging #243 into master will increase coverage by 0.11%.
The diff coverage is 94%.

@@            Coverage Diff             @@
##           master     #243      +/-   ##
==========================================
+ Coverage   87.05%   87.16%   +0.11%     
==========================================
  Files         180      180              
  Lines       11181    11160      -21     
==========================================
- Hits         9734     9728       -6     
+ Misses       1205     1190      -15     
  Partials      242      242
Impacted Files Coverage Δ
Libplanet/Blocks/Block.cs 84.51% <100%> (-0.86%) ⬇️
Libplanet.Tests/Tx/TxFixture.cs 100% <100%> (ø) ⬆️
Libplanet/Blockchain/BlockChain.cs 95.98% <100%> (-0.17%) ⬇️
Libplanet.Tests/Tx/TransactionTest.cs 93.37% <84.61%> (ø) ⬆️
Libplanet.Tests/Blocks/BlockTest.cs 97.85% <86.2%> (+0.12%) ⬆️
Libplanet.Tests/Net/SwarmTest.cs 93.4% <0%> (+0.47%) ⬆️
Libplanet/Net/Swarm.cs 77.8% <0%> (+0.89%) ⬆️
/// right after all <see cref="Transaction{T}.Actions"/> of
/// <see cref="Transactions"/>, which maintains the changes from
/// the states of the previous <see cref="Block{T}"/>.</returns>
/// <returns>An <see cref="ActionEvaluation{T}"/> for each steps.

This comment has been minimized.

Copy link
@dahlia

dahlia May 17, 2019

Member
Suggested change
/// <returns>An <see cref="ActionEvaluation{T}"/> for each steps.
/// <returns>An <see cref="ActionEvaluation{T}"/> for each step.

This comment has been minimized.

Copy link
@dahlia

dahlia May 17, 2019

Member

This sounds ambiguous too.

This comment has been minimized.

Copy link
@dahlia

dahlia May 17, 2019

Member

It would be fine if we just copy a <returns> docs field of Transaction<T>.EvaluateActionsGradually() method and adjust some words.

Show resolved Hide resolved CHANGES.md Outdated
Show resolved Hide resolved Libplanet.Tests/Blocks/BlockTest.cs Outdated
Show resolved Hide resolved Libplanet.Tests/Tx/TxFixture.cs Outdated
Show resolved Hide resolved Libplanet.Tests/Tx/TxFixture.cs Outdated
Show resolved Hide resolved Libplanet.Tests/Blocks/BlockTest.cs Outdated
Show resolved Hide resolved Libplanet.Tests/Blocks/BlockTest.cs Outdated
Hash,
Index,
delta,
Miner.Value);

This comment has been minimized.

Copy link
@dahlia

dahlia May 17, 2019

Member

Is this just a style adjustment?

This comment has been minimized.

Copy link
@longfin

longfin May 17, 2019

Author Member

Yes. and I also convert var to explicit type(IEnumerable<ActionEvaluation<T>>).

Come to think of it, it would be better if triples is renamed to evaluations.

/// which represents the final states and maintains the changes
/// from the states of the previous <see cref="Block{T}"/>.</para>
/// <para>Otherwise it returns an <see cref="ActionEvaluation{T}"/>
/// for each steps.</para>

This comment has been minimized.

Copy link
@dahlia

dahlia May 17, 2019

Member
Suggested change
/// for each steps.</para>
/// for each step.</para>

This comment has been minimized.

Copy link
@dahlia

dahlia May 17, 2019

Member

By the way, “step” here is ambiguous if it means an action or a transaction.

This comment has been minimized.

Copy link
@longfin

longfin May 17, 2019

Author Member

'for each IAction' seems to be appropriate. I'll edit.

/// <para>Otherwise it returns an <see cref="IAccountStateDelta"/>
/// which represents the final states and maintains the changes
/// from the states of the previous <see cref="Block{T}"/>.</para>
/// <para>Otherwise it returns an <see cref="ActionEvaluation{T}"/>

This comment has been minimized.

Copy link
@dahlia

dahlia May 17, 2019

Member
Suggested change
/// <para>Otherwise it returns an <see cref="ActionEvaluation{T}"/>
/// <para>Otherwise it enumerates an <see cref="ActionEvaluation{T}"/>

longfin and others added some commits May 17, 2019

Apply suggestions from code review
Co-Authored-By: Hong Minhee <hong.minhee@gmail.com>
@dahlia

dahlia approved these changes May 17, 2019

@longfin longfin merged commit a9f0945 into planetarium:master May 17, 2019

10 checks passed

Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
changelog This contains self-describing changelog.
Details
codecov/patch 94% of diff hit (target 87.05%)
Details
codecov/project 87.16% (+0.11%) compared to dacef83
Details
planetarium.libplanet Build #20190517.5 succeeded
Details
planetarium.libplanet (Linux) Linux succeeded
Details
planetarium.libplanet (Windows) Windows succeeded
Details
planetarium.libplanet (Windows_coverage) Windows_coverage succeeded
Details
planetarium.libplanet (macOS) macOS succeeded
Details
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.