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

Action exceptions should be recorded as a part of blockchain #860

Closed
dahlia opened this issue May 4, 2020 · 6 comments
Closed

Action exceptions should be recorded as a part of blockchain #860

dahlia opened this issue May 4, 2020 · 6 comments
Assignees
Labels
suggestion Suggestion or feature request

Comments

@dahlia
Copy link
Contributor

dahlia commented May 4, 2020

Due to the limitation of the current IAction API, validation logic tends to be duplicated (one in the Execute() method and other one in the game UI code). If exceptions raised by actions are recorded as a part of blockchain, such duplicated validation code in the game UI side can be replaced by reusing the result/error code of an action.

@dahlia
Copy link
Contributor Author

dahlia commented May 18, 2020

@ipdae Any ideas?

@ipdae
Copy link
Member

ipdae commented May 18, 2020

In my opinion, it would be nice to be able to retrieve the exception that occurs when executing the action in the game through an interface such as EvaluationOuputStates.Exception.

@longfin longfin added the suggestion Suggestion or feature request label May 19, 2020
@longfin longfin self-assigned this May 19, 2020
@longfin
Copy link
Member

longfin commented May 20, 2020

In my opinion, it would be nice to be able to retrieve the exception that occurs when executing the action in the game through an interface such as EvaluationOuputStates.Exception.

ActionEvaluation is public class but the game using Libplanet doesn't access it directly. Instead, the game does through IAction.Render().

so there are several options if we want to allow to access generated exception to users.

  • give whole ActionEvaluation as .Render()'s argument instead of IActionContext and IAccountStateDelta.
  • add exception to .Render()'s arguments.
  • attach it to IAccountStateDelta or IActionContext.

The former is more natural to me. Is there any idea? @planetarium/libplanet @ipdae

@dahlia
Copy link
Contributor Author

dahlia commented May 20, 2020

How about adding a new distinct method to IAction besides Render()/Unrender(), e.g., RenderError()/UnrenderError()? IAction would look like:

public interface IAction
{
    IAccountStateDelta Execute(IActionContext context);
    void Render(IActionContext context, IAccountStateDelta nextStates);
    void Unrender(IActionContext context, IAccountStateDelta nextStates);
    void RenderError(IActionContext context, Exception exception);    // 🆕
    void UnrenderError(IActionContext context, Exception exception);  // 🆕
    IValue PlainValue { get; }
    void LoadPlainValue(IValue plainValue);
}

@longfin
Copy link
Member

longfin commented May 20, 2020

How about adding a new distinct method to IAction besides Render()/Unrender(), e.g., RenderError()/UnrenderError()? IAction would look like:

public interface IAction
{
    IAccountStateDelta Execute(IActionContext context);
    void Render(IActionContext context, IAccountStateDelta nextStates);
    void Unrender(IActionContext context, IAccountStateDelta nextStates);
    void RenderError(IActionContext context, Exception exception);    // 🆕
    void UnrenderError(IActionContext context, Exception exception);  // 🆕
    IValue PlainValue { get; }
    void LoadPlainValue(IValue plainValue);
}

RenderError() / UnrenderError() seem good. I'll take them.

@longfin
Copy link
Member

longfin commented Jun 4, 2020

It seems resolved by #875

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
suggestion Suggestion or feature request
Projects
None yet
Development

No branches or pull requests

3 participants