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

fix: remove payable check for non-mutable functions #774

Closed
wants to merge 1 commit into from

Conversation

austinabell
Copy link
Contributor

@austinabell austinabell commented Mar 31, 2022

Opinionated change, probably shouldn't come in, opening for discussion. It was a foot gun for what I was just doing, but it doesn't seem super clean to just remove.

Pros:

  • Removes foot gun of trying to call a method without parameters as a view call and failing

Cons:

  • Silently removes non-payable check to anyone using this method and modifying state manually

Neutral:

@MaksymZavershynskyi
Copy link
Contributor

Is this a backwards-compatible change from the perspective of API or behavior?

@austinabell
Copy link
Contributor Author

Is this a backwards-compatible change from the perspective of API or behavior?

Are you referring to the change in this draft or the proposed protocol change?

The draft PR change is backwards compatible from the API surface, but just would remove the payable check by default. Not going to open this PR up, was created more for discussion.

@austinabell
Copy link
Contributor Author

Also, I have a terrible memory, because there was already an issue for this: #718

@austinabell
Copy link
Contributor Author

Closing this because the logic in nearcore was changed in near/nearcore#8433

The better solution now is to just have the non-payable check on all methods, and only remove if the attribute payable is applied.

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

Successfully merging this pull request may close these issues.

None yet

2 participants