Restrict actions and states types

[moreal]

This pr is related with #541.

• Restrict types of IAction.PlainValue to Bencodex.Types.IValue.

[dahlia]

This file seems unnecessary to be changed.

[dahlia]

Still the diff remains. Should be unchanged.

[codecov]

Codecov Report

Merging #552 into master will decrease coverage by 0.33%.
The diff coverage is 88.28%.

@@            Coverage Diff             @@
##           master     #552      +/-   ##
==========================================
- Coverage   90.85%   90.52%   -0.34%     
==========================================
  Files         201      202       +1     
  Lines       15300    15315      +15     
==========================================
- Hits        13901    13864      -37     
- Misses       1107     1163      +56     
+ Partials      292      288       -4

Impacted Files	Coverage Δ
Libplanet.Tests/Action/ActionContextTest.cs	90.47% <ø> (ø)	⬆️
Libplanet.Tests/Common/Action/BaseAction.cs	50% <ø> (ø)	⬆️
Libplanet.Tests/Common/Action/DetectRehearsal.cs	0% <0%> (ø)	⬆️
Libplanet.Tests/Common/Action/BattleResult.cs	41.07% <100%> (-23.38%)	⬇️
Libplanet/Tx/RawTransaction.cs	82.35% <100%> (-2.32%)	⬇️
Libplanet.Tests/Store/StoreTest.cs	99.06% <100%> (ø)	⬆️
Libplanet/Tx/Transaction.cs	94.48% <100%> (-0.11%)	⬇️
Libplanet/Action/IAccountStateDelta.cs	100% <100%> (ø)	⬆️
Libplanet/Net/Messages/RecentStates.cs	100% <100%> (ø)	⬆️
Libplanet/Store/LiteDBStore.cs	88.48% <100%> (ø)	⬆️
... and 27 more

[dahlia]

We need to update example source code here and there so that they are compatible to the new interface.

libplanet/Libplanet/Action/IAction.cs

Lines 43 to 206 in 7b617fd

	/// <code><![CDATA[
	/// using System;
	/// using System.Collections.Immutable;
	/// using Libplanet;
	/// using Libplanet.Action;
	///
	/// public class MyAction : IAction
	/// {
	/// // Declare an enum type to distinguish types of in-game logic.
	/// public enum ActType { CreateCharacter, Attack, Heal }
	///
	/// // Declare properties (or fields) to store "bound" argument values.
	/// public ActType Type { get; private set; }
	/// public Address TargetAddress { get; private set; }
	///
	/// // Action must has a public parameterless constructor.
	/// // Usually this is used only by Libplanet's internals.
	/// public MyAction() {}
	///
	/// // Take argument values to "bind" through constructor parameters.
	/// public MyAction(ActType type, Address targetAddress)
	/// {
	/// Type = type;
	/// TargetAddress = targetAddress;
	/// }
	///
	/// // The main game logic belongs to here. It takes the
	/// // previous states through its parameter named context,
	/// // and is offered "bound" argument values through
	/// // its own properties (or fields).
	/// IAccountStateDelta IAction.Execute(IActionContext context)
	/// {
	/// // Gets the state immediately before this action is executed.
	/// // ImmutableDictionary<string, uint> is just for example,
	/// // As far as it is serializable, you can store any types.
	/// // (We recommend to use immutable types though.)
	/// var state = (ImmutableDictionary<string, uint>)
	/// context.PreviousStates.GetState(TargetAddress);
	///
	/// // This variable purposes to store the state
	/// // right after this action finishes.
	/// ImmutableDictionary<string, uint> nextState;
	///
	/// // Does different things depending on the action's type.
	/// // This way is against the common principals of programming
	/// // as it is just an example. You could compare this with
	/// // a better example of PolymorphicAction<T> class.
	/// switch (Type)
	/// {
	/// case ActType.CreateCharacter:
	/// if (!TargetAddress.Equals(context.Signer))
	/// throw new Exception(
	/// "TargetAddress of CreateCharacter action " +
	/// "only can be the same address to the " +
	/// "Transaction<T>.Signer.");
	/// else if (!(state is null))
	/// throw new Exception(
	/// "Character was already created.");
	///
	/// nextState = ImmutableDictionary<string, uint>.Empty
	/// .Add("hp", 20);
	/// break;
	///
	/// case ActType.Attack:
	/// nextState =
	/// state.SetItem("hp", Math.Max(state["hp"] - 5, 0));
	/// break;
	///
	/// case ActType.Heal:
	/// nextState =
	/// state.SetItem("hp", Math.Min(state["hp"] + 5, 20));
	/// break;
	///
	/// default:
	/// throw new Exception(
	/// "Properties are not properly initialized.");
	/// }
	///
	/// // Builds a delta (dirty) from previous to next states, and
	/// // returns it.
	/// return context.PreviousStates.SetState(TargetAddress,
	/// nextState);
	/// }
	///
	/// // Side effects, i.e., any effects on other than states, are
	/// // done here.
	/// void IAction.Render(
	/// IActionContext context,
	/// IAccountStateDelta nextStates)
	/// {
	/// Character c;
	///
	/// // You could compare this with a better example of
	/// // PolymorphicAction<T> class.
	/// switch (Type)
	/// {
	/// case ActType.CreateCharacter:
	/// c = new Character
	/// {
	/// Address = TargetAddress,
	/// Hp = 0,
	/// };
	/// break;
	///
	/// case ActType.Attack:
	/// case ActType.Heal:
	/// c = Character.GetByAddress(TargetAddress);
	/// break;
	///
	/// default:
	/// break;
	/// }
	///
	/// c?.Hp = nextStates.GetState(TargetAddress)["hp"];
	/// c?.Draw();
	/// }
	///
	/// // Sometimes a block to which an action belongs can be
	/// // a "stale." If that action already has been rendered,
	/// // it should be undone.
	/// void IAction.Unrender(
	/// IActionContext context,
	/// IAccountStateDelta nextStates)
	/// {
	/// Character c = Character.GetByAddress(TargetAddress);
	///
	/// // You could compare this with a better example of
	/// // PolymorphicAction<T> class.
	/// switch (Type)
	/// {
	/// case ActType.CreateCharacter:
	/// c.Hide();
	/// break;
	///
	/// case ActType.Attack:
	/// case ActType.Heal:
	/// IAccountStateDelta prevStates = context.PreviousStates;
	/// c.Hp = prevStates.GetState(TargetAddress)["hp"];
	/// c.Draw();
	/// break;
	///
	/// default:
	/// break;
	/// }
	/// }
	///
	/// // Serializes its "bound arguments" so that they are transmitted
	/// // over network or stored to the persistent storage.
	/// // It uses .NET's built-in serialization mechanism.
	/// IImmutableDictionary<string, object> IAction.PlainValue =>
	/// ImmutableDictionary<string, object>.Empty
	/// .Add("type", Type)
	/// .Add("target_address", TargetAddress);
	///
	/// // Deserializes "bound arguments". That is, it is inverse
	/// // of PlainValue property.
	/// void IAction.LoadPlainValue(
	/// IImmutableDictionary<string, object> plainValue)
	/// {
	/// Type = (ActType)plainValue["type"];
	/// TargetAddress = (Address)plainValue["target_address"];
	/// }
	/// }
	/// ]]></code>

libplanet/Libplanet/Action/PolymorphicAction.cs

Lines 27 to 187 in 7b617fd

	/// <code><![CDATA[
	/// using System;
	/// using System.Collections.Immutable;
	/// using Libplanet;
	/// using Libplanet.Action;
	///
	/// // Instead of having multiple in-game actions in a class,
	/// // in this example, we declare one abstract base class
	/// // and its three concrete subclasses.
	/// public abstract class ActionBase : IAction
	/// {
	/// public ActionBase() { }
	///
	/// public ActionBase(Address targetAddress)
	/// {
	/// TargetAddress = targetAddress;
	/// }
	///
	/// public Address TargetAddress { get; private set; }
	///
	/// // Leaves Execute() abstract so that concrete subclasses
	/// // implement their own logic.
	/// public abstract IAccountStateDelta Execute(IActionContext context);
	///
	/// // Makes Render() no-op by default, but overrideable by subclasses.
	/// public virtual void Render(
	/// IActionContext context,
	/// IAccountStateDelta nextStates)
	/// {
	/// }
	///
	/// // Makes Unrender() no-op by default,
	/// // but overrideable by subclasses.
	/// public virtual void Unrender(
	/// IActionContext context,
	/// IAccountStateDelta nextStates)
	/// {
	/// }
	///
	/// IImmutableDictionary<string, object> IAction.PlainValue =>
	/// ImmutableDictionary<string, object>.Empty
	/// .Add("target_address", TargetAddress);
	///
	/// void IAction.LoadPlainValue(
	/// IImmutableDictionary<string, object> plainValue)
	/// {
	/// TargetAddress = (Address)plainValue["target_address"];
	/// }
	/// }
	///
	/// // PolymorphicAction<T> requires concrete action classes marked with
	/// // ActionTypeAttribute.
	/// // There is only one required parameter to ActionTypeAttribute,
	/// // which takes a unique identifier of the action type.
	/// // This is used for serialization and deserialization under the hood.
	/// [ActionType("create_character")]
	/// public sealed class CreateCharacter : ActionBase
	/// {
	/// public override IAccountStateDelta Execute(IActionContext context)
	/// {
	/// var state = (ImmutableDictionary<string, uint>)
	/// context.PreviousStates.GetState(TargetAddress);
	/// if (!TargetAddress.Equals(context.Signer))
	/// throw new Exception(
	/// "TargetAddress of CreateCharacter action only can be " +
	/// "the same address to the Transaction<T>.Signer."
	/// );
	/// else if (!(state is null))
	/// throw new Exception("Character was already created.");
	/// return context.PreviousStates.SetState(
	/// TargetAddress,
	/// ImmutableDictionary<string, uint>.Empty.Add("hp", 20)
	/// );
	/// }
	///
	/// void IAction.Render(
	/// IActionContext context,
	/// IAccountStateDelta nextStates)
	/// {
	/// var c = new Character
	/// {
	/// Address = TargetAddress,
	/// Hp = nextStates.GetState(TargetAddress)["hp"],
	/// };
	/// c.Draw();
	/// break;
	/// }
	///
	/// void IAction.Unrender(
	/// IActionContext context,
	/// IAccountStateDelta nextStates)
	/// {
	/// Character c = Character.GetByAddress(TargetAddress);
	/// c.Hide();
	/// }
	/// }
	///
	/// [ActionType("attack")]
	/// public sealed class Attack : ActionBase
	/// {
	/// public override IAccountStateDelta Execute(IActionContext context)
	/// {
	/// var state = (ImmutableDictionary<string, uint>)
	/// context.PreviousStates.GetState(TargetAddress);
	/// return context.PreviousStates.SetState(
	/// TargetAddress,
	/// state.SetItem("hp", Math.Max(state["hp"] - 5, 0))
	/// );
	/// }
	///
	/// void IAction.Render(
	/// IActionContext context,
	/// IAccountStateDelta nextStates)
	/// {
	/// Character c = Character.GetByAddress(TargetAddress);
	/// c.Hp = nextStates.GetState(TargetAddress)["hp"];
	/// c.Draw();
	/// }
	///
	/// void IAction.Unrender(
	/// IActionContext context,
	/// IAccountStateDelta nextStates)
	/// {
	/// Character c = Character.GetByAddress(TargetAddress);
	/// c.Hp = context.PreviousStates.GetState(TargetAddress)["hp"];
	/// c.Draw();
	/// }
	/// }
	///
	/// [ActionType("heal")]
	/// public sealed class Heal : ActionBase
	/// {
	/// public override IAccountStateDelta Execute(IActionContext context)
	/// {
	/// var state = (ImmutableDictionary<string, uint>)
	/// context.PreviousStates.GetState(TargetAddress);
	/// return context.PreviousStates.SetState(
	/// TargetAddress,
	/// state.SetItem("hp", Math.Min(state["hp"] + 5, 20))
	/// );
	/// }
	///
	/// void IAction.Render(
	/// IActionContext context,
	/// IAccountStateDelta nextStates)
	/// {
	/// Character c = Character.GetByAddress(TargetAddress);
	/// c.Hp = nextStates.GetState(TargetAddress)["hp"];
	/// c.Draw();
	/// }
	///
	/// void IAction.Unrender(
	/// IActionContext context,
	/// IAccountStateDelta nextStates)
	/// {
	/// Character c = Character.GetByAddress(TargetAddress);
	/// c.Hp = context.PreviousStates.GetState(TargetAddress)["hp"];
	/// c.Draw();
	/// }
	/// }
	/// ]]></code>

[dahlia]

There seem two ways to convert a string object to a Text object: casting operator and .ToBencodex() method, and in this code two ways coexist. Is there any guideline to choose the appropriate way for each case between two ways?

Personally, .ToBencodex() has a shortcoming that it hides the resulting type; although we readers could find it's something IValue, it's difficult to guess its concrete type (e.g., Text, not just IValue).

[moreal]

1. I fixed all cases like (Text)"string" and I'll prepare the guideline.
2. Yes, so I tried to rename the names like AsText, AsInteger and AsBinary etc..

[dahlia]

So, what's the point of having v.AsText() instead of (Text)v?

[moreal]

In my thought, v.AsText() looks better than (Text)v.

[dahlia]

Still the diff remains. Should be unchanged.

[dahlia]

This explanation is outdated, right?

[moreal]

Yes, I will update it.

[dahlia]

This explanation is also outdated. Should be rewritten.

[dahlia]

Why is this key so special?

[moreal]

Also IMO: all the objects like Block, Transaction, should be serialized with Bencodex for scalability to other languages and avoid the situation like BencodexFormatter.cs#L65

[dahlia]

“To write readable code” seems a too broad description (i.e., provides no much information).

Suggested change

	- Added `BencodexExtension` to write more readable code.
	- Added `BencodexExtension` static class.

[dahlia]

So, what's the point of having v.AsText() instead of (Text)v?

[dahlia]

@moreal I'd like to do pair programming on this patch next week. Is it alright?

[moreal]

@moreal I'd like to do pair programming on this patch next week. Is it alright?

Of course, it seems essential because it is too huge.