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 BlockChain<T>.UnstageTransactions() #223

Merged
merged 1 commit into from Apr 29, 2019

Conversation

Projects
None yet
3 participants
@earlbread
Copy link
Member

commented Apr 29, 2019

This patch adds BlockChain<T>.UnstageTransactions() method.

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

@earlbread earlbread self-assigned this Apr 29, 2019

@earlbread earlbread force-pushed the earlbread:unstage-transactions branch from a741851 to 037ccb3 Apr 29, 2019

@codecov

This comment has been minimized.

Copy link

commented Apr 29, 2019

Codecov Report

Merging #223 into master will decrease coverage by 0.21%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #223      +/-   ##
==========================================
- Coverage   84.08%   83.87%   -0.22%     
==========================================
  Files          75       75              
  Lines        3488     3492       +4     
==========================================
- Hits         2933     2929       -4     
- Misses        555      563       +8
Impacted Files Coverage Δ
Libplanet/Blockchain/BlockChain.cs 98.04% <100%> (+0.02%) ⬆️
Libplanet/Net/Swarm.cs 79.68% <0%> (-0.9%) ⬇️
@dahlia

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

What would this method be used for?

@earlbread

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

What would this method be used for?

We need this method to avoid duplicate rewards in games.

@dahlia

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

Could you rebase this on the current master?

@earlbread earlbread force-pushed the earlbread:unstage-transactions branch from 037ccb3 to 73ed2d0 Apr 29, 2019

@earlbread

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

I rebased this on the master branch. @dahlia

@earlbread earlbread force-pushed the earlbread:unstage-transactions branch 3 times, most recently from 2b79a7b to c300520 Apr 29, 2019

@earlbread

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

I rebased again. Could you please review again? @dahlia @longfin

foreach (Transaction<T> tx in transactions)
{
Transactions.Remove(tx.Id);
}

This comment has been minimized.

Copy link
@dahlia

dahlia Apr 29, 2019

Member

@longfin Should we remove these transactions from pool as well?

This could break things, for example:

Block<Action> b = Block<Action>.Mine(..., transactions: txs);
blockChain.Append(b);
blockChain.UnstageTransactions(txs);
Block<Action> b2 = blockChain.Blocks[b.blockHash];  // This will crash.

This comment has been minimized.

Copy link
@longfin

longfin Apr 29, 2019

Member

Yes, it can be problem. and, I think that UnstageTransactions() should remove only staged transactions, not mined transaction.

This comment has been minimized.

Copy link
@earlbread

earlbread Apr 29, 2019

Author Member

You're right. So should we make like BlockChain<T>.UnstageTransactionIds() instead of BlockChain<T>.UnstageTransactions()?

This comment has been minimized.

Copy link
@dahlia

dahlia Apr 29, 2019

Member

The current name looks better to me, because it exposes less implementation details.

@earlbread earlbread force-pushed the earlbread:unstage-transactions branch from c300520 to 9b8c5d4 Apr 29, 2019

@earlbread earlbread force-pushed the earlbread:unstage-transactions branch from 9b8c5d4 to 256974d Apr 29, 2019

@earlbread

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

I made BlockChain<T>.UnstageTransactionIds() method remove only staged transactions, not mined transaction. Please take a look. @dahlia @longfin

@dahlia

dahlia approved these changes Apr 29, 2019

@earlbread earlbread merged commit dd95b02 into planetarium:master Apr 29, 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 100% of diff hit (target 84.08%)
Details
codecov/project Absolute coverage decreased by -0.21% but relative coverage increased by +15.91% compared to 34538b7
Details

@earlbread earlbread deleted the earlbread:unstage-transactions branch Apr 29, 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.