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

Set function access in Json ABI based on presence of ctx and self parameter #783

Merged
merged 10 commits into from
Dec 3, 2022

Conversation

vuvoth
Copy link
Contributor

@vuvoth vuvoth commented Sep 2, 2022

What was wrong?

Resolve #678

How was it fixed?

To-Do

  • OPTIONAL: Update Spec if applicable

  • Add entry to the release notes (may forgo for trivial changes)

  • Clean up commit history

Copy link
Member

@Y-Nak Y-Nak left a comment

Choose a reason for hiding this comment

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

mut would be launched soon. So I think it'd be better to finish the PR after its deployment.

Comment on lines 34 to 35
has_self: bool,
has_ctx: bool,
Copy link
Member

Choose a reason for hiding this comment

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

It's better to take simply state_mutability instead of introducing Fe-specific concepts like self and ctx to avoid unnecessary tight coupling with Fe language specification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mut would be launched soon. So I think it'd be better to finish the PR after its deployment.

Okay. I got it!

@sbillig
Copy link
Collaborator

sbillig commented Nov 23, 2022

mut would be launched soon. So I think it'd be better to finish the PR after its deployment.

"soon" has finally come, a couple months late 😅. I didn't update the json abi in the mut PR, @vuvoth.

@vuvoth
Copy link
Contributor Author

vuvoth commented Nov 23, 2022

mut would be launched soon. So I think it'd be better to finish the PR after its deployment.

"soon" has finally come, a couple months late sweat_smile. I didn't update the json abi in the mut PR, @vuvoth.

Thanks ~~. I will update it on my PR!

@sbillig
Copy link
Collaborator

sbillig commented Nov 29, 2022

Sorry @vuvoth, I just removed CtxDecl because it seemed pointless. Didn't mean to make extra work for you 😬

@vuvoth
Copy link
Contributor Author

vuvoth commented Dec 1, 2022

Sorry @vuvoth, I just removed CtxDecl because it seemed pointless. Didn't mean to make extra work for you grimacing

No problem!

// |none self | self | mut self |
// | none ctx | pure | view | payable/nonpayable |
// | ctx | view | view | payable/nonpayable |
// | mut ctx | payable/nopayable | view | payable/nonpayable |
Copy link
Member

@g-r-a-n-t g-r-a-n-t Dec 2, 2022

Choose a reason for hiding this comment

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

@cburgdorf mut ctx + self should be payable/nonpayable, right?

Copy link
Collaborator

@cburgdorf cburgdorf Dec 2, 2022

Choose a reason for hiding this comment

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

Yes, as soon as we have mut in a public contract function, then it needs to be payable (we don't have nonpayable)

Copy link
Contributor Author

@vuvoth vuvoth Dec 2, 2022

Choose a reason for hiding this comment

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

I think more sense if we only allow mut ctx follow after mut self because we want to modify something and this should be payable/nopayable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These are two different things. You can take self non mutable but take mut ctx because e.g. you want to emit an event. So you aren't changing any storage of the contract but you are still making changes to the blockchain. In terms of the ABI both of these should be marked as payable. There's a longer form write-up of the different access classes under that scheme: #558

@sbillig
Copy link
Collaborator

sbillig commented Dec 2, 2022

@vuvoth I pushed a commit to your branch, changing the Option<bool> args to enums, and using a match statement instead of an if/else tree. I also removed some changes that weren't necessary (unused functions, etc), and added a newsfragment.

@g-r-a-n-t g-r-a-n-t self-requested a review December 3, 2022 06:33
@Y-Nak
Copy link
Member

Y-Nak commented Dec 3, 2022

CI book fails because the request to alchemy returns 403. I'll fix it in another PR.

@Y-Nak Y-Nak merged commit a069051 into ethereum:master Dec 3, 2022
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.

Set function access in Json ABI based on presence of ctx and self parameter
5 participants