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

Clarify Frame::HostFunction #150

Open
jonjove opened this issue Jun 30, 2022 · 5 comments
Open

Clarify Frame::HostFunction #150

jonjove opened this issue Jun 30, 2022 · 5 comments

Comments

@jonjove
Copy link
Contributor

jonjove commented Jun 30, 2022

What problem does your feature solve?

The name here confuses me. I don't understand the intention. Based on the code, it looks like we are only using Frame::HostFunction as the initial frame when calling in via invoke_function (this behavior is fine). But the name suggests that we would push a Frame::HostFunction every time a host function is called, which we are not doing (this behavior is also fine). Perhaps we can come up with a name that indicates the actual usage?

What would you like to see?

A better name that indicates the actual usage.

What alternatives are there?

Leave the name as is, with some comments about what it actually means. I don't feel great about this though.

@smac711
Copy link

smac711 commented Sep 13, 2022

@jonjove will this need to be completed ahead of Meridian or can we de prioritize this work into a later iteration or just backlog it for now in now iteration?

@jonjove
Copy link
Contributor Author

jonjove commented Sep 13, 2022

Backlog it, it's purely about code quality and is not observable to users.

@smac711
Copy link

smac711 commented Sep 16, 2022

@jonjove moving this to backlog to be prioritized into a later sprint.

@jayz22
Copy link
Contributor

jayz22 commented Jun 28, 2023

I get the confusion but the Frame definition is purely internal so maybe we just leave it as is and add some documentation in the code. Also this is also fairly engrained with the InvokeHostFunctionOp (https://soroban.stellar.org/docs/getting-started/invoking-contracts-with-transactions#invokehostfunctionop) definitions, which means entering the Host environment via one specific host function with three possible actions (invoke contract /create contract/update wasm), which is the basis of the soroban transactions.

Given this is hard to change, and the benefit is heavily out-weighted by potential confusion and adaptation cost. I'll move this to post-testnet to add better code documentations in the host.

@graydon
Copy link
Contributor

graydon commented Jul 6, 2023

@jayz22 I noticed this again while working with the Frame type recently and agree with Jon's position that it's too easy to read this as meaning "any time a host function is invoked". I would like to propose changing it to Frame::InitialInvokeHostFunctionOp or Frame::TopLevelInvokeHostFunctionOp or Frame::OutermostInvokeHostFunctionOp or something like that, for clarity.

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

No branches or pull requests

5 participants
@graydon @jayz22 @jonjove @smac711 and others