prommiseResult throws an error

[volovyks]

We should not mimic the Rust design here. Throwing an error is the standard behavior for JS.
The same goes for the return type. I do not want to add {error, result} because it would be the only place in the whole API where we use such a design.

[ailisp]

Nice! Didn't think of that, the throwing-Error or return the success value just beautifully fit!

[gagdiez]

@ailisp @volovyks I would still argue in favour of having a {status, result}, as JS promises do, since they are simpler to explain to developers.

Lets compare a simple workflow, where the callback returns the result value:

result structure

let response = near.promiseResult();

if(response.status === success){
  log("it worked")
  return response.value
}else{
  log("it didn't work because:", response.error)
  return
}

try/catch

try{
  let result = near.promiseResult(); // if this fails the catch arises
  log("it worked")
  return result
}catch(e){
  log("it didn't work because:", e.message)
  return
}

I personally find the first logic simpler to read and explain for devs.

[gagdiez]

@ailisp @volovyks we could even use the status pending, fulfilled and rejected for the promise result, as JS promises, and the result can be a value, an error, or undefined: https://www.w3schools.com/js/js_promise.asp

[volovyks]

I think that the concept of {status, value} is more confusing. The developer will need to check the function signature, or literally read the source code if he is using JS.

Let's not forget that try{} catch() {} is optional and you can just

return near.promiseResult() // bad practice, use try-catch!

Looks like we need 1-2 more opinions in order to make a decision. @BenKurrek @doriancrutcher @petarvujovic98

[ailisp]

I vote this PR's throw behavior. Even if the developer forget to write try-catch, the throw will cause contract panic and there's not going to be a security issue. Also, throwing an error is more common practice in JavaScript libraries than return {error, value}

[volovyks]

Ok, merging. We can always come back to this question if developers start complaining.

[gagdiez]

@ailisp @volovyks I was thinking yesterday, and there is another thing I don't like about the try/catch pattern:
it will catch any error, and not only the callback failure.

try{
  let result = near.promiseResult(); // if this fails the catch arises
  // but if I have a bug/error here also!
}

If I introduced a bug in my callback method (for example, accessing the wrong index), it will be masked by the try/catch.

This can be even dangerous in the classic situation where:

1. the user calls a method in my contract attaching money, I make state changes
2. I make a XCC sending money
3. I check if it failed in the callback, and rollback state + send money back if necessary

If my callback has an error, the code will act as if the xcc failed and send money back to the user.

The solution to that is to isolate the try/catch, and check later if the promise succeeded:

let result: type = undefined;
let xcc_succeeded: bool = false;
try{
  let result = near.promiseResult(); // if this fails the catch arises
  xcc_succeeded = true;
} catch(e){ }

if (succeeded){
  // it succeeded: do something
}else{
  // it failed: rollback
}

In that way, we:

1. Make sure the rollback executes if and only if xcc failed.
2. Do not mask internal errors: errors/bugs will panic with the correct error message.

But this is a lot of boilerplate.

Because of this, I think the try/catch patter can be error-prone, and it would be simpler and safer to use a simple structure with a {status, result}.

[osalkanovic]

Maybe having both options can cover all cases
Example is here

[petarvujovic98]

I think that the { status, result } pattern is pretty common in JS. For reference the standard fetch method has this for HTTP requests, so developers are pretty familiar with this.

I also think that the throw does feel more JS-like, but not all JS features are good, even if they are common practice. So I would vote for the { status, result } pattern here like @gagdiez.