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 IBlockPolicy.IsTransactionValid #827

Merged
merged 6 commits into from
Mar 19, 2020

Conversation

earlbread
Copy link
Contributor

This adds IBlockPolicy.IsValidTransaction method which is a method to determine if a transaction is valid.

@codecov
Copy link

codecov bot commented Mar 18, 2020

Codecov Report

Merging #827 into master will increase coverage by 0.06%.
The diff coverage is 99.28%.

@@            Coverage Diff             @@
##           master     #827      +/-   ##
==========================================
+ Coverage   87.38%   87.44%   +0.06%     
==========================================
  Files         243      244       +1     
  Lines       22156    22288     +132     
==========================================
+ Hits        19360    19489     +129     
  Misses       1453     1453              
- Partials     1343     1346       +3     
Impacted Files Coverage Δ
Libplanet.Tests/Blockchain/BlockChainTest.cs 98.73% <98.03%> (-0.03%) ⬇️
Libplanet.Tests/Blockchain/NullPolicy.cs 88.88% <100.00%> (+1.38%) ⬆️
...lanet.Tests/Blockchain/Policies/BlockPolicyTest.cs 97.57% <100.00%> (+0.24%) ⬆️
Libplanet.Tests/Net/SwarmTest.cs 96.55% <100.00%> (-0.04%) ⬇️
Libplanet/Blockchain/BlockChain.cs 90.47% <100.00%> (+0.20%) ⬆️
Libplanet/Blockchain/Policies/BlockPolicy.cs 84.31% <100.00%> (+0.98%) ⬆️
Libplanet/Net/Swarm.cs 84.73% <100.00%> (+0.04%) ⬆️
Libplanet/Tx/TxViolatingBlockPolicyException.cs 100.00% <100.00%> (ø)
Libplanet/Net/Protocols/KBucket.cs 85.59% <0.00%> (-2.55%) ⬇️
... and 3 more

Libplanet/Blockchain/BlockChain.cs Outdated Show resolved Hide resolved
Libplanet/Blockchain/Policies/IBlockPolicy.cs Outdated Show resolved Hide resolved
Libplanet/Blockchain/BlockChain.cs Outdated Show resolved Hide resolved
@earlbread earlbread changed the title Add IBlockPolicy.IsValidTransaction Add IBlockPolicy.IsTransactionValid Mar 19, 2020
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
@@ -13,6 +14,8 @@ namespace Libplanet.Blockchain.Policies
public class BlockPolicy<T> : IBlockPolicy<T>
where T : IAction, new()
{
private readonly Predicate<Transaction<T>> _isTransactionValid;
Copy link
Contributor

Choose a reason for hiding this comment

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

IsTransactionInvalid method just calls _isTransactionsValid and pass its return value. so how about using property?

Suggested change
private readonly Predicate<Transaction<T>> _isTransactionValid;
public Predicate<Transaction<T>> IsTransactionValid { get; }

Copy link
Contributor

Choose a reason for hiding this comment

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

@moreal I believe the way you suggested does not satisfy the interface anyway, because in the IL level delegate properties and methods are merely distinct each other; the same looking syntax between delegate properties and methods in C# is just a coincidence.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dahlia If I had understood correctly, it means that method bool IsTransactionValid(Transaction<T>) and delegate property Predicate<Transaction<T>> are distinct each other so it will not be achieved.

If then, it was what I really wanted to suggest to replace IBlockPolicy><T>.IsTransactionValid(Transaction<T>) with Predicate<Transaction<T>>. As code, it will be like below.

public interface IBlockPolicy<T>
    where T : IAction, new()
{
    // bool IsTransactionValid(Transaction<T> transaction);
    Predicate<Transaction<T>> IsTransactionValid { get; }
}

Copy link
Contributor

@dahlia dahlia Mar 19, 2020

Choose a reason for hiding this comment

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

@moreal That was how this patch looked before it's amended, but then it became like the current way in a reason: .NET interfaces say nothing about constructors, so an interface anyway cannot enforce its implementations to take a delegate. On the other hand, it's unclear what's benefit of having a predicate as a delegate property instead of a method, which is more common and natural.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I understood. Thanks for your good explanation.

@earlbread earlbread force-pushed the validate-transaction branch 2 times, most recently from 014f992 to 96e8a24 Compare March 19, 2020 07:25
CHANGES.md Outdated Show resolved Hide resolved
longfin
longfin previously approved these changes Mar 19, 2020
@earlbread earlbread requested a review from longfin March 19, 2020 07:42
@earlbread earlbread merged commit 890abfc into planetarium:master Mar 19, 2020
@earlbread earlbread deleted the validate-transaction branch March 19, 2020 08:40
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.

4 participants