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

[Neo Framework]add IWithdrawable #1056

Merged
merged 3 commits into from
Jun 7, 2024
Merged

Conversation

Jim8y
Copy link
Contributor

@Jim8y Jim8y commented Jun 5, 2024

This pr standarize the withdrawable interface and add its comments.

@Jim8y Jim8y requested a review from a team June 5, 2024 04:16
/// <summary>
/// Interface of method that indicate a contract can be withdrawn.
/// </summary>
public interface IWithdrawable
Copy link
Member

Choose a reason for hiding this comment

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

Iverificable? Verify is not only for withdraw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for IWithdrawable, not for verify. Just verify happens to be IWithdrawable method.

For the ease of helping developers to implement contract that is withdrawable, not a contract that is verificable.

Copy link
Member

Choose a reason for hiding this comment

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

But it won't be better if we create IVerify? that it can be used for multiple things, like withdraw GAS and NEO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well yes, but if its purpose is withdrawing, why call it verify?

Copy link
Contributor Author

@Jim8y Jim8y Jun 6, 2024

Choose a reason for hiding this comment

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

@shargon my suggestion is create another pr that create IVerify, then IWithdrawable inherits IVerify, but this pr will still focus on IWithdrawable, cause its name corresponds to INEP11Payable/INEP17Paybel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shargon Go ahead with what you want. But please make sure to have detailed comments for IWithdrawable.

@Jim8y Jim8y requested a review from a team June 6, 2024 03:26
@Jim8y Jim8y merged commit cbae713 into neo-project:master Jun 7, 2024
3 checks passed
@Jim8y Jim8y deleted the withdrawable branch June 7, 2024 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants