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

Frames are inconsistent in tests and invoked WASM #485

Open
leighmcculloch opened this issue Sep 26, 2022 · 5 comments
Open

Frames are inconsistent in tests and invoked WASM #485

leighmcculloch opened this issue Sep 26, 2022 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@leighmcculloch
Copy link
Member

I've noticed in my testing that the frames that are setup for the call stack are different in a test vs when WASM is being executed.

There's only one frame when directly invoking a test contract in an SDK test.

There are two frames when the contract is invoked directly as a WASM contract.

@leighmcculloch leighmcculloch added the bug Something isn't working label Sep 26, 2022
@jonjove
Copy link
Contributor

jonjove commented Sep 27, 2022

This is caused by the additional Frame::HostFunction that is generated by Host::invoke_function. The x86-test builds don't go through this call path because it was designed for usage from stellar-core. It is probably possible to go through this call path if you want to, though.

@leighmcculloch
Copy link
Member Author

Got it. So to summarize, in the test case we are calling Env::call which adds just a contract to the stack, meaning the stack is increased in size by 1, where-as a real top-level invocation calls Host::invoke_function which adds a Frame::HostFunction to the stack before adding the contract call, meaning the stack is increased in size by 2.

For reference this is the code that dances around this inconsistency:

let st = match frames.as_slice() {
// There are always two frames when WASM is executed in the VM.
[.., f2, _] => match f2 {
#[cfg(feature = "vm")]
Frame::ContractVM(_, _) => Ok(InvokerType::Contract),
Frame::HostFunction(_) => Ok(InvokerType::Account),
Frame::Token(id, _) => Ok(InvokerType::Contract),
#[cfg(feature = "testutils")]
Frame::TestContract(_, _) => Ok(InvokerType::Contract),
},
// In tests contracts are executed with a single frame.
// TODO: Investigate this discrepancy: https://github.com/stellar/rs-soroban-env/issues/485.
[f1] => match f1 {
#[cfg(feature = "vm")]
Frame::ContractVM(_, _) => {
Err(self.err_general("contract vm found in last frame which is unexpected"))
}
Frame::HostFunction(_) => Ok(InvokerType::Account),
Frame::Token(id, _) => {
Err(self.err_general("token found in last frame which is unexpected"))
}
#[cfg(feature = "testutils")]
Frame::TestContract(_, _) => Ok(InvokerType::Account),
},
_ => Err(self.err_general("no frames to derive the invoker from")),

@leighmcculloch
Copy link
Member Author

leighmcculloch commented Sep 27, 2022

A possible solution is to change the SDK generated contract client to call Host::invoke_contract instead of Env::call in the case that it is a top-level invocation, however, we share the same code between top-level calls and sub-level calls, and I think I don't think we should introduce fragmented behavior in the SDK. It would require the user learning two APIs.

@leighmcculloch
Copy link
Member Author

Another possible solution is to remove the Frame::HostFunction variant, and not insert a host function frame on direct invocations. From best I can tell the only things that are reliant on its presence are the functions that get the contract or account who is the invoker, and these things don't need the frame to exist to do that.

@graydon Wdyt?

@jonjove
Copy link
Contributor

jonjove commented Sep 27, 2022

Tangentially related to #150 since removing Frame::HostFunction would resolve that issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants
@graydon @leighmcculloch @jonjove and others