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

Refactor the interface for actions updating states #121

Merged
merged 19 commits into from Mar 15, 2019

Conversation

Projects
2 participants
@dahlia
Copy link
Member

dahlia commented Mar 8, 2019

This patch retouched IAction and Transaction<T> APIs to make library users easier to implement their own IAction classes. Two key changes are:

  • Transaction<T>'s SenderRecipient model was replaced by SignerUpdatedAddresses model. Unlike cryptocurrencies, transactions in games are not necessarily a transfer of assets, so it is difficult to determine what type of assert is transferred or who will receives the asset. A more useful perspective is, like what kind of transformation is performed, or what states are changed.

    To be close to this perspective, we decided to get rid of Transaction<T>.Recipient (which is singular) and have Transaction<T>.UpdatedAddresses (which is plural) instead. As there is no more asset to transfer, the term Sender was also renamed to Signer, which fits more to the new perspective.

  • The Addresses IAction tries to update no more need to be manually coded using IAction.RequestStates() method. That method was removed at all, and updated Addresses became automatically scanned using dirty-tracking for the most cases.

More detailed changes are written in CHANGES.md file.


Tasks

  • BlockChain needs to validate if all Transaction<T>.UpdatedAddresses match to the actual addresses affected by Transaction<T>.Actions. Note that as it depends on precedent states, cannot be done by Transaction<T> in itself.
  • Block<T>.Validate() had been not validating its Transactions. Transaction<T>.Validate() also should be called inside that method.
  • Currently UpdatedAddresses must be coded by human when a Transaction<T> is instantiated. There should be a façade factory to play the given actions and automatically determines the updated addresses.
  • Get rid of the whole AddressStateMap thing? It no differs from IImmutableDictionary<Address, object>. See also #98.
  • Changelogs should be more organized and the whole change need to be explained at a higher level.

@dahlia dahlia force-pushed the dahlia:advance-tx-recipient branch from a0b8476 to e63bb08 Mar 8, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 8, 2019

Codecov Report

Merging #121 into master will decrease coverage by 3%.
The diff coverage is 92.96%.

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
- Coverage   88.14%   85.14%   -3.01%     
==========================================
  Files          96       99       +3     
  Lines        3593     3715     +122     
==========================================
- Hits         3167     3163       -4     
- Misses        426      552     +126
Impacted Files Coverage Δ
Libplanet/Store/TransactionSet.cs 96.77% <ø> (ø) ⬆️
Libplanet/Tx/TxId.cs 88.88% <ø> (ø) ⬆️
...planet/Serialization/BencodexFormatterConverter.cs 20.33% <0%> (-1.48%) ⬇️
Libplanet/Tx/InvalidTxPublicKeyException.cs 100% <100%> (ø) ⬆️
Libplanet/Tx/Transaction.cs 95.97% <100%> (+1.05%) ⬆️
Libplanet/Action/IAccountStateDelta.cs 100% <100%> (ø)
Libplanet/Tx/InvalidTxSignatureException.cs 100% <100%> (ø) ⬆️
Libplanet/Store/BlockSet.cs 96.77% <100%> (ø) ⬆️
Libplanet/Blockchain/BlockChain.cs 98.43% <100%> (-0.77%) ⬇️
Libplanet/Address.cs 93.58% <100%> (ø) ⬆️
... and 17 more

@longfin longfin added this to In progress in comuka-20190309 via automation Mar 9, 2019

@dahlia dahlia force-pushed the dahlia:advance-tx-recipient branch 2 times, most recently from 5ee39ab to 2541840 Mar 9, 2019

@longfin longfin added this to the 0.2.0 milestone Mar 11, 2019

@dahlia dahlia force-pushed the dahlia:advance-tx-recipient branch 5 times, most recently from a811ad6 to 6b50a15 Mar 11, 2019

@dahlia dahlia force-pushed the dahlia:advance-tx-recipient branch from 27b4a54 to 5622f3c Mar 14, 2019

@dahlia dahlia changed the title 🚧 Refactor the interface for actions updating states Refactor the interface for actions updating states Mar 14, 2019

@dahlia dahlia requested a review from longfin Mar 14, 2019

@dahlia dahlia self-assigned this Mar 14, 2019

@dahlia

This comment has been minimized.

Copy link
Member Author

dahlia commented Mar 14, 2019

@longfin Could you review this?

@@ -3,6 +3,7 @@
using System.Collections.Immutable;
using System.Drawing.Design;
using System.Linq;
using System.Runtime.CompilerServices;

This comment has been minimized.

@longfin

longfin Mar 14, 2019

Member

Is this using statement necessary?

This comment has been minimized.

@dahlia

dahlia Mar 14, 2019

Author Member

No, it's mistaken. I will remove them!

updatedAddresses,
ts,
actionsArray
).EvaluateActions(

This comment has been minimized.

@longfin

longfin Mar 14, 2019

Member

It's important to point out that IAction.Execute() is being executed in Transaction<T>.Create().

This comment has been minimized.

@dahlia

dahlia Mar 14, 2019

Author Member

It's for filling out UpdatedAddresses automatically, and documented in <remarks> section.

@dahlia dahlia force-pushed the dahlia:advance-tx-recipient branch from 8dcb47e to e848600 Mar 14, 2019

@dahlia dahlia force-pushed the dahlia:advance-tx-recipient branch from e848600 to cca67a1 Mar 14, 2019

@dahlia

This comment has been minimized.

Copy link
Member Author

dahlia commented Mar 15, 2019

@longfin I amended a commit to fix the build failure on Windows. Can you approve this again?

@dahlia dahlia merged commit 352a783 into planetarium:master Mar 15, 2019

2 checks passed

codecov/patch 92.96% of diff hit (target 88.14%)
Details
codecov/project Absolute coverage decreased by -3% but relative coverage increased by +4.82% compared to 6cb9223
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.