Add support for sendTransaction error fetching

[zivkovicmilos]

Description

This PR adds support for broadcast_tx_sync error fetching in sendTransaction (Provider method).

Based on discussions in #46, it is worth mentioning that his error check will only catch base-level transaction errors (or rather, throw them to the caller).

Additionally, this PR also introduces typed errors for transaction sends.

Resolves #46

cc @jinoosss

[zivkovicmilos]

@jinoosss
What do you think about adding support for sendTransaction to be able to execute broadcast_tx_sync, ex with a flag?

[jinoosss]

@zivkovicmilos
I apologize for the late response.

In my opinion, if there's an option needed for execution, it's whether to call it with 'broadcast_tx_async' or 'broadcast_tx_sync'.

And I would like to ask your opinion on throwing a clear error that can identify the type.

Personally, I thought we needed something that would clearly identify the error.
I also thought that when an error occurs, the client should be able to tell which error occurred.
Like an AxiosError or an error in Ether.

It would also be nice to distinguish between the source of the error and the type of error.

// Error codes
export type ErrorCode = 'TX_DECODE_ERROR' | 'UNKNOWN_ERROR' | ...;


// Custom error objects
export class TM2Error extends Error {
  public readonly code: ErrorCode;

  constructor(code: ErrorCode, name: string, message: string) {
    super(message);
    this.code = code;
    this.name = name;
  }
}

export class ABCIError extends TM2Error {
  constructor(code: ErrorCode, message: string) {
    super(code, 'ABCIError', message);
  }

  static createTxDecodeError(): ABCIError {
    return new ABCIError('TX_DECODE_ERROR', 'tx decode error');
  }
}

// Utility functions for errors
export function makeABCIError(abciErrorKey: string): ABCIError {
  ...
}

[zivkovicmilos]

@jinoosss

In my opinion, if there's an option needed for execution, it's whether to call it with 'broadcast_tx_async' or 'broadcast_tx_sync'.

Understood, so you think we should expand the API to give users this ability to select what kind of broadcast they want? The default would of course be an async broadcast, but if you pass in the param (ex. flag) it would be sync.

Personally, I thought we needed something that would clearly identify the error.
I also thought that when an error occurs, the client should be able to tell which error occurred.
Like an AxiosError or an error in Ether.

I also love this idea, but it seems tricky to implement with the way the gno node currently returns transaction errors:

• you only get the type
• you get the full error trace in the Log

I guess what we can do is map internally (in the library) the std errors, to specific types we can control. For example, if the error type is /std.UnauthorizedError, we know that the error type is UnauthorizedError and that the message should be unauthorized error. We can also include the stack trace if you think it's relevant.

What do you think?

[jinoosss]

@zivkovicmilos
I completely agree with you.
I think it will be better at handling errors.

Thanks for thinking about this.

[zivkovicmilos]

Hey @jinoosss,

I've added support for the broadcast mode selection, now you can choose between sync (the default) and commit:
fda2201

Obviously there is error checking for each mode type.

Let me know what you think about the API

[jinoosss]

@zivkovicmilos Thank you for this work.

Using the API is explicit and looks good.
It seems to do the behavior I'm currently expecting.

I have one question.
Is there any issue with using the broadcast_tx_commit endpoint in a production environment rather than a testnet?
There is something in the GNO RPC endpoint comments.
(https://github.com/gnolang/gno/blob/master/tm2/pkg/bft/rpc/core/mempool.go#L163-L258)

Or are there any plans for the broadcast_tx_commit endpoint to change?

[zivkovicmilos]

I have one question.
Is there any issue with using the broadcast_tx_commit endpoint in a production environment rather than a testnet?
There is something in the GNO RPC endpoint comments.
(https://github.com/gnolang/gno/blob/master/tm2/pkg/bft/rpc/core/mempool.go#L163-L258)
Or are there any plans for the broadcast_tx_commit endpoint to change?

The comments that you linked are really outdated, they were from a time when you could query the transaction using the node events, which have since been deprecated. Currently there is no way for you to tap into these sorts of events, but support for it is coming soon.
The reason commit is discouraged is :

• because there was the ability to track the transaction status (this doesn't exist anymore)
• because it forces the client to wait until the transaction is committed, if ever

I like having the ability for the user to specify in the API what kind of call they want (and if they don't care, then it defaults to a broadcast sync)

I'll update this PR to include the better error handling we've discussed, that's why it's still in draft 🙏

[jinoosss]

Thanks for the reply.
I will forward to your work.

[zivkovicmilos]

@jinoosss

I've updated this PR to include the typed errors we discussed:

71fc2c7

Please let me know if this looks good to you, and we can merge it out 🚀

EDIT: I apologize it took this long to get the required changes in, I've managed to get a handle on context switching tasks 🙏

[jinoosss]

@zivkovicmilos Thanks for the good work.

It will definitely make it easier to handle errors.
It's nice to be able to differentiate between detailed errors within Tm2Error.

I left a small comment about the exception cases for error handling.
I would be grateful if you could let me know your thoughts.

[jinoosss]

Thank you for your work.