RFC: Transaction Flow (parity of behavior between localBlockchain and Testnet)

[rpanic]

Motivation & Current State

The sending of Transactions and subsequent status retrieval is inconsistent between local and remote implementations and additionally re-uses the TransactionId and re-uses properties for different functionality.
As an example, in remote networks, TransactionId.isSuccess is used for both the indication that the sending of the transaction has succeeded and does not work correctly after calling .wait(). See #819
In comparison, LocalBlockchain throws at .send() if the transaction cannot be applied to the ledger, and does not implement .wait().

Altough the interface for these implementation is the same, the behaviour is completely different and leads to worse developer experience and extra effort to check for those things in scripts.

Proposed changes

I propose an extension to the current API which aligns both implementations and seperates concerns into different classes.

Transaction.send() : Promise<TransactionId>

Where TransactionId uniformly implements properties as:

• isSuccess: The Transaction is well-formed and passes initial checks by the node (I think the mina node primarily checks some nonce preconditions and the fee payer)
• errors: Error[] that occurred
• hash(): Returns the hash of the txid
• wait(): Waits for the tx to be mined (details below)

TransactionId.wait() : Promise<IncludedTransaction>

The first implementation spec only needs an default mina node

• isIncluded: The tx got included in a block (is only false if errors occurred)
• errors: Error[] that occurred

The second possibility would extend the above and involve the new archive-node api:

• getInfo() : Promise<IncludedTransactionInfo>: Retrieves information about the mined transaction from an archive node.
  IncludedTransactionInfo would include inclusion-time errors, block info. This method would throw if no archive node url has been specified for remote networks.
  This should be an extra method in order to allow operation without an archive-node api, as is the current interface for remote networks.

Reliably usage would depend on Archive-Node-API#69 being implemented.

Things I am not sure about

hash() is currently taken from the graphql response. I don't know why but it seems that this should be possible to do in snarkyjs

[mitschabaude]

Thanks for the proposal @rpanic! I like it a lot:

• Making the two interfaces consistent will solve a huge pain point for users
• The additional info returned on IncludedTransaction is an important addition that enables applications to show the full transaction lifecycle to their users, from being sent to being successfully included, and to debug any errors that may happen in that lifecycle.

I have a few comments:

Errors

The change from Transaction.send() throwing an error on LocalBlockchain to not throwing an error breaks most existing SnarkyJS tests. Also, in circumstances like tests, letting it throw an error can be more ergonomic than having to inspect the returned errors array. Throwing an error makes it easier for us to give helpful feedback to the user, such as the message shown when encountering the Invalid_fee_excess error.

Therefore, I propose to add an alternative method which retains the old behavior (of throwing an error), and implement this method on both LocalBlockchain and Network. It would be a wrapper around Transaction.send() which inspects the errors array and possibly throws an error. Not sure how to call it, maybe

• Transaction.sendException()?
• Transaction.sendOrThrow()?

TransactionId.wait()

Minor comment: Let's rename TransactionId to something clearer, like PendingTransaction.

It's unclear to me how .wait() is supposed to work on local blockchain. I would advocate to keep it simple and make .wait() return immediately and not do something fancy like simulating blocks.

One question/thought: There may be a difference in what errors the Mina node returns from GraphQL (subset of errors that can occur in the transaction logic, but some additional checks done before entering the transaction pool) vs what the local blockchain returns from .send() (all possible errors that can occur in the transaction logic, but may miss some other checks). How important is it that these are consistent? Some errors that the local blockchain throws on send() may only show up on the Network after wait(). We would have to work a bit on LocalBlockchain to make it more similar to the real network in that respect.

hash() is currently taken from the graphql response. I don't know why but it seems that this should be possible to do in snarkyjs

Yes, we can implement hash() in SnarkyJS. It's already part of mina-signer which shares a lot of code with SnarkyJS.

[rpanic]

Thanks for your comments!

I like the idea of adding a method that would throw if errors occurred. I would go with Transaction.sendOrThrow() or even Transaction.sendThrowing(), sendException seems a bit misleading to me.

TransactionId.wait()

Renaming makes a lot of sense!
I agree, .wait() should return resolve immediately, but I think we can remove the warning it outputs at the moment.
In the medium-term simulating blocks and keeping track of network-states (especially things that we can use in preconditions) would be great for testing to get as close as possible to real conditions, but that is out of scope for this one I think.

There may be a difference in what errors the Mina node returns from GraphQL ....

I think that ideally we can also make both implementations consistent, but that kind of comes in tandem with the simulation argument above. It makes no sense to check for things that we don't keep track of in the localblockchain.

I think even if there is some discrepancy of errors returned on .send() vs. .wait() the additional effort required to test on both networks is dramatically reduced through these changes.

[jasongitmail]

@Trivo25 could you take a look at this too please? Important enough to have a couple reviewers. After your conference.

[Trivo25]

No further comments to add. I think it would be great to get the tx e2e flow for both the LocalBlockchain and the live Network up to sync. Ideally, without simulating blocks on the JS side (imo).

[nicc]

Prioritised. First step is to make choices as peresented by this RFC, copy it over to the RFC repo, and present it to architects and community. The work will follow from there.

[MartinMinkov]

This comment is a continuation of what was proposed earlier in this issue. I will attempt to make a finalization on all the choices for the work that is to follow.

Proposed changes

In an effort to consolidate the API between the local blockchain and remote blockchain APIs, I propose we make the following changes:

Add PendingTransaction and IncludedTransaction types

We currently use a TypeScript interface called TransactionId. This is a structure that's defined in this file. This structure is used to represent a transaction that is either pending or included in a blockchain and is created after we call transaction.send().

I propose we replace TransactionId with two types: PendingTransaction and IncludedTransaction. These types will represent pending transactions and be included in a blockchain, respectively.

PendingTransaction

The current TransactionId will be renamed to PendingTransaction. This type will be used to represent pending transactions in the blockchain. It has the following structure:

interface PendingTransaction {
  isSuccess: boolean;
  wait(options?: { maxAttempts?: number; interval?: number }): Promise<void>;
  hash(): string | undefined;
  // data and errors are not defined, but are present in the remote blockchain API
  data?: any;
  errors?: any;
}

These properties must match between the local and remote blockchain APIs. The following behaviour is expected:

• isSuccess will be true if the transaction is included in the blockchain, and false if not.
• wait will wait for the transaction to be included in the blockchain. If the transaction is included in the blockchain, it will resolve. If the transaction is not included in the blockchain after the maximum number of attempts, it will be rejected. For the local blockchain API, this will be implemented using a Promise that resolves immediately if the transaction is successful and rejects otherwise. For the remote blockchain API, the behaviour will be kept the same as the current implementation (which uses polling).
• hash will return the hash of the transaction. The local blockchain does not expose it, but the remote blockchain does. We will need to expose it in the local blockchain API as well. This can be done quickly by either porting the hashing code to TypeScript or exposing a function in local_ledger.ml that returns the hash of the transaction via hash_command

One oddity about PendingTransaction is that on the Network API, it sneaks in data and errors properties that is not present in the Local API. Because PendingTransaction is supposed to be used in both the local and remote settings, we should add these properties to the PendingTransaction type officially and stub out the values for the local version. This will make the API consistent and allow developers to use the same code for both the local and remote blockchain APIs.

The type that PendingTransaction should have is:

type PendingTransaction = Pick<
  Transaction,
  'transaction' | 'toJSON' | 'toPretty'
> & {
  isSuccess: boolean;
  wait(options?: { maxAttempts?: number; interval?: number }): Promise<void>;
  hash(): string;
  data?: Fetch.SendZkAppResponse; // This will be defined in fetch.ts
  errors?: string[];
};

IncludedTransaction

This type will be used to represent transactions that are included in the blockchain. It has the following structure:

type IncludedTransaction = Pick<
  Transaction,
  'transaction' | 'toJSON' | 'toPretty'
> & {
  isIncluded: boolean;
  errors: string[];
};

These properties must match between the local and remote blockchain APIs. The following behavior is expected:

• isIncluded will be true if the transaction is included in the blockchain, and false if it is not.
• errors will be an array of strings representing the errors during the transaction. This will be an empty array if the transaction was successful.

Add getTransactionInfo function to IncludedTransaction

As pointed out by @rpanic, we should add a getTransactionInfo function to IncludedTransaction that returns the transaction info. This will be a simple function that returns the following information:

• blockHeight: The height of the block that the transaction was included in.
• blockHash: The hash of the block that the transaction was included in.
• parentHash: The hash of the parent block of the block that the transaction was included in.
• globalSlot: The global slot of the block that the transaction was included in.
• timestamp: The timestamp of the block that the transaction was included in.

For the local blockchain, this information can be stubbed out with empty strings, since local blockchain does not have blocks. For the remote blockchain, this information can be fetched from the Archive-Node-API.
This query is currently not supported and must be added to the Archive-Node-API. The lift for this is low, as the information is already available in the Archive-Node-API, and we just need to expose it by its GraphQL schema. If the endpoint for a Archive-Node-API is unavailable, we simply throw an error and log it to the console.

This cannot be done by the Mina daemon GraphQL endpoint as there's no query I'm aware of that will return block information given a transaction hash.

Clone Transaction properties to PendingTransaction and IncludedTransaction

We should clone some of the properties of Transaction to PendingTransaction and IncludedTransaction. The reasoning being, if we create a PendingTransaction/IncludedTransaction from a Transaction, we should be able to get all the information about the transaction from the TransactionId. Not doing so would mean that we would have to keep a reference to the Transaction object, which is not ideal.

When we create a transaction using o1js by calling Mina.Transaction, we get a Transaction object back. The Transaction object has the following structure:

type Transaction = {
    transaction: ZkappCommand;
    toJSON(): string;
    toPretty(): any;
    toGraphqlQuery(): string;
    sign(additionalKeys?: PrivateKey[] | undefined): Transaction;
    prove(): Promise<...>;
    send(): Promise<...>;
}

We should add the following properties to PendingTransaction and IncludedTransaction:

• transaction: This will be the transaction object that was used to create the PendingTransaction or IncludedTransaction.
• toJSON: This will be a function that returns the JSON representation of the transaction.
• toPretty: This will be a function that returns a pretty representation of the transaction.

This keeps the API consistent and allows us to get all the information about the transaction from the PendingTransaction or IncludedTransaction.

type PendingTransaction = Pick<
  Transaction,
  'transaction' | 'toJSON' | 'toPretty'
> & {
  isSuccess: boolean;
  wait(options?: { maxAttempts?: number; interval?: number }): Promise<void>;
  hash(): string;
  data?: Fetch.SendZkAppResponse;
  errors?: string[];
};

type IncludedTransaction = Pick<
  PendingTransaction,
  'transaction' | 'toJSON' | 'toPretty'
> & {
  isIncluded: boolean;
  errors: string[];
  getTransactionInfo(): Promise<TransactionInfo>;
};

Testing

Our testing goals are to ensure that the LocalBlockchain and Network APIs match in behaviour, that the PendingTransaction and IncludedTransaction types are working as expected, and prevent any regressions.

Ensure LocalBlockchain and Network APIs match in behaviour

The LocalBlockchain and Network APIs should match in behaviour. This means that functions should return the exact shape of data and have the same behaviour. This is extremely important as a developer often writes their zkApp to target Localblockchain and then deploys it to the Network. If the APIs are inconsistent, the developer must rewrite their deploy/run code to target the Network API. This is not ideal and should be avoided.

An audit of the APIs should be done to ensure that the APIs match in behaviour. Both the local and remote versions are defined in mina.ts, so thsese should be considered. If there are any discrepancies, they should be fixed.

To test the behaviour of the APIs, we will write an integration test that combines the LocalBlockchain and Network APIs. The test will be run alongside other integration tests and will use lightnet for it's remote target. The test will deploy a zkApp to the LocalBlockchain and then deploy the same zkApp to the Network. We will then test the behaviour of each API and ensure they produce the same results.

Add documentation for Mina interface

The documentation for the Mina interface is currently lacking. We should add documentation for the Mina interface and all the functions and types it exposes. This will make it easier for developers to use the o1js library. We can also add examples for each function and type to make it easier for developers to understand how to use the library. Both the local and remote blockchain APIs should have the same documentation since our efforts in this RFC is to consolidate the functionality. An example of where this would be incredibly helpful is the difference of usage between fetch and get APIs for events/actions.

[mitschabaude]

Very nice @MartinMinkov, I'm excited for this to land!

I only have a comment on clarifying the behaviour of send() / wait(). I think for consistency it should be like this:

// does not throw
Transaction.send(): Promise<PendingTransaction>;

// throws if and only if `.send()` would have an `.errors` property; otherwise same behaviour
Transaction.sendOrThrow(): Promise<PendingTransaction>; 

// does not throw
PendingTransaction.wait(): Promise<IncludedTransaction>;

// throws if and only if `.wait()` would have an `.errors` property; otherwise same behaviour
PendingTransaction.waitOrThrow(): Promise<IncludedTransaction>;

comment on the name xyzOrThrow(): I don't think it's super pretty, but we should come up with some convention. @MartinMinkov feel free to change it to something you like better.

In Ocaml there is a very consistent convention of having xyz_exn : unit -> 't vs xyz : unit -> 't result, which creates a good DX: you know the error behaviour of something without trying it out.

[nicc]

Thanks @MartinMinkov. Looks great! Would it be possible to extend the testing plan a bit? You already do this for the section on parity between LocalBlockchain and Network APIs and I think it'd be good to cover what tests we'd expect overall. You don't have to get down to individual tests necessrily. This section of the template might be helpful. The intention is for RFCs to inform acceptance criteria / definition of done and should as such include tests 🙏

[MartinMinkov]

Sounds good! I added a Testing section under the comment. The work involved is writing an integration test that will use lightnet (similar to other places in our integration tests) and comparing both APIs for the same behaviour. This will help us catch regressions if the transaction API changes.