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

Add host function for aborting current subtransaction with details / a status code #146

Closed
graydon opened this issue Jun 28, 2022 · 12 comments
Assignees
Labels
enhancement New feature or request hostfunction Work on specific host functions

Comments

@graydon
Copy link
Contributor

graydon commented Jun 28, 2022

We need a way for a contract to exit the subtransaction / call that it's currently running immediately, without popping / returning all the way back out to its outermost frame. The moral equivalent of the exit(int) standard library function in C.

Pass void for success, or a status for failure -- or just a status if we're going to keep the "ok" / "success" status code (this might be the exception to the "never pass a status" rule).

We should allow aborting the current subtransaction with some details -- eg. calling env.abort(status) -- rather than just calling panic!(). This might include revising the status type to include a variant that denotes its file name / line number / a user-provided message as in #205.

@graydon graydon added enhancement New feature or request hostfunction Work on specific host functions labels Jun 28, 2022
@leighmcculloch
Copy link
Member

leighmcculloch commented Jul 6, 2022

Cross-posting this comment from Discord:

I don't think we should do this.

I don't think we should set up contract developers to terminate contracts successfully in this way. Ideally contract functions can only succeed by exiting the top level function at a return point. Otherwise it becomes difficult to ascertain all the possible paths a contract can succeed. Any code a contract pulls in could inject an exit(success) and cause a contract call not to error which could be exploited. Any reader of a contract has to search for every exit to figure out if a contract can exit successfully in a variety of edge cases.

For the failure case there is already a way to do this which I think we should continue to use: by calling panic!, or using the other helper methods like .expect or .unwrap on a type. All of these methods provide meaningful information when run in tests on developers machines, and then are optimized away when built with wasm.

@jonjove
Copy link
Contributor

jonjove commented Jul 7, 2022

Agreed with @leighmcculloch, it seems safer not to do this.

@graydon
Copy link
Contributor Author

graydon commented Jul 13, 2022

I disagree with, but accept, the consensus-dislike of multiple success-exits (I think they're more useful than they cost).

I still think we need a fail-with-user-provided-details call. The panic!() calls in the existing contract are problematically uninformative to anyone trying to debug them.

@graydon graydon changed the title Add host function for exiting current subtransaction with either success or failure Add host function for aborting current subtransaction with details / a status code Jul 13, 2022
@leighmcculloch
Copy link
Member

The panic!() calls in the existing contract are problematically uninformative to anyone trying to debug them.

They're informative when running locally in a debugger, but aren't useful after they've been compiled to wasm.

Would there be a way to bundle other information into panic that isn't a string, such as a Status?

@leighmcculloch
Copy link
Member

The main reason we started using a panic! instead of our own fns that trap was because the wasm unreachable fn isn't a const fn, but we can put panic! in const fns.

@graydon
Copy link
Contributor Author

graydon commented Jul 13, 2022

Yeah, in places we need it I understand. I just mean like reading through @jonjove's contract there are a bunch of logic-panics that will return a very opaque error to the user.

@leighmcculloch
Copy link
Member

In the Rust std lib there exists std::panic::panic_any, which can be used to pass any value to the panic handler, even non-strings. This could be used to pass a Status that we surface somehow. But I can't find it in core even though rust-lang/rust#78500 and rust-lang/rust#80162 seem to indicate that panic should be identical between std and core 🤔.

@jonjove
Copy link
Contributor

jonjove commented Jul 13, 2022

@graydon I haven't tried to do reasonable error reporting in the contracts I'm writing since error reporting is still in flux. I definitely don't want to give the impression that what's there is desirable to me.

@graydon
Copy link
Contributor Author

graydon commented Jul 15, 2022

We have (as of yesterday actually hooked-up) a way to log an error. So we could almost get away with a pure SDK-side env.log(msg); panic!(); but it won't convey a status code to the caller. I think I want something a tiny bit richer, like env.fail_with_status(status) so that the user code can say a handleable status that gets back to its caller. WDYT?

@leighmcculloch
Copy link
Member

leighmcculloch commented Jul 15, 2022

Hmm, how would this fit in with generating debug events? This looks like a different interface. We almost achieve the same thing with panic + Status returns, since a user contract could return a Status. We also need logs/debugevents independent of panic since we probably want to log debug events with failing conversions that don't panic.

@graydon
Copy link
Contributor Author

graydon commented Aug 11, 2022

@jonjove now that you've done some work on making the token "native", does it make more sense to you to have a abort_current_contract_with_status(Status) function?

@jonjove
Copy link
Contributor

jonjove commented Aug 11, 2022

Let's revisit that once I merge the native contract, so that everyone can see what is there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hostfunction Work on specific host functions
Projects
None yet
Development

No branches or pull requests

4 participants