Nuklai / hyperchain-js-sdk

(deprecated)
MIT License
1 stars 1 forks source link

[Bug] Missing states for `getTransactionInfo` #7

Open piotrpietruszka opened 1 month ago

piotrpietruszka commented 1 month ago

Problem

At the moment of the call we know that transaction exists, but instead of returning pending state it is treated as not found.

Current flow

  1. Creating TX:
    
    {"jsonrpc":"2.0","id":3,"method":"hypersdk.submitTx","params":{"tx":[0,0,1,145,235,183,177,144,140,20,20,91,11,0,136,246,255,252,176,181,159,97,108,66,253,41,120,140,137,190,69,93,152,204,243,68,100,165,225,120,0,0,0,0,0,0,1,110,1,0,0,216,54,63,160,11,17,23,232,137,131,146,62,75,98,255,80,250,116,192,103,243,130,187,162,21,225,197,204,115,246,66,245,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,15,66,64,0,0,0,0,0,214,6,142,254,183,99,198,222,39,92,31,22,227,18,246,148,210,147,219,159,71,74,233,124,155,200,147,216,127,98,19,230,101,109,166,45,98,103,147,210,206,202,207,71,132,172,191,10,154,202,110,101,237,184,173,209,125,150,122,132,140,145,242,236,233,3,51,218,7,59,173,200,21,176,50,222,151,38,25,191,154,194,128,228,161,108,225,75,152,203,122,120,80,249,100,10]}}

{ "jsonrpc": "2.0", "result": { "txId": "2BXbsTS4K2Era4aXiPSipaPS8aWrRYTatcSmtFxz8onqSrBbU6" }, "id": 3 }


2. Fetching TX (which is not true, we know that this exists):

{"jsonrpc":"2.0","id":40,"method":"nuklaivm.tx","params":{"txId":"2BXbsTS4K2Era4aXiPSipaPS8aWrRYTatcSmtFxz8onqSrBbU6"}}

{ "jsonrpc": "2.0", "error": { "code": -32000, "message": "tx not found", "data": null }, "id": 40 }

3. After 5s fetching  TX:

{"jsonrpc":"2.0","id":68,"method":"nuklaivm.tx","params":{"txId":"2BXbsTS4K2Era4aXiPSipaPS8aWrRYTatcSmtFxz8onqSrBbU6"}}

{ "jsonrpc": "2.0", "result": { "timestamp": 1726236555200, "success": true, "units": [ 224, 7, 14, 50, 26 ], "fee": 321 }, "id": 68 }


## Expected
In a response pending state should be distinct. 

1. Creating TX:

{ "jsonrpc": "2.0", "result": { "txId": "2BXbsTS4K2Era4aXiPSipaPS8aWrRYTatcSmtFxz8onqSrBbU6" }, "id": 3 }

or better unify responses (it's very natural that create transaction endpoint will return transaction model):

{ "jsonrpc": "2.0", "result": { "timestamp": 1726236555200, "status": "CREATED", <- NEW FIELD "units": [ ... ], "fee": 321 }, "id": 68 }

2. Fetching TX (pending):

{ "jsonrpc": "2.0", "result": { "timestamp": 1726236555200, "status": "PENDING", <- NEW FIELD "units": [ ... ], "fee": 321 }, "id": 68 }


3. After 5s fetching  TX:

{ "jsonrpc": "2.0", "result": { "timestamp": 1726236555200, "status": "SUCCESS", <- NEW FIELD "units": [ ... ], "fee": 321 }, "id": 68 }

kpachhai commented 1 month ago

Note that transaction id is generated entirely offline so even if you get the id after u send the transaction, it doesn't necessarily mean it's successful. It takes around 5 seconds for the tx to be finalized hence only after 5 seconds or so, you get the result about the id.

Our sdk basically interacts with the blockchain rpc node to fetch the info about a tx id which is why it says tx not found because the tx has not been finalized.

Or, were u suggestion we make the change what you described at the js sdk level to show it's in pending state? If so, this could be done. Though, if the tx fails, the rpc API would still return it as tx not found error however we could still form the custom failure response at the sdk level again if we want to.

piotrpietruszka commented 1 month ago

Note that transaction id is generated entirely offline so even if you get the id after u send the transaction, it doesn't necessarily mean it's successful.

Describing the state as PENDING, SUCCESS, FAILD sounds reasonable.

Our sdk basically interacts with the blockchain rpc node to fetch the info about a tx id which is why it says tx not found because the tx has not been finalized.

Though, if the tx fails, the rpc API would still return it as tx not found error however we could still form the custom failure response at the sdk level again if we want to.

From UI, design perspective we are sharing TX ID directly after the creation. For example to check TX in a ledger. Why only finalised TXs can be found? Failed, pending TXs are stored off-chain?

Or, were u suggestion we make the change what you described at the js sdk level to show it's in pending state? If so, this could be done.

Solving this on SDK level can be temporary solution. But keep in mind that even for failed transaction we need similar amount of details as for success one to display to user.

kpachhai commented 1 month ago

Ah I see what you are getting at. Currently, we do not store failed transactions on ledger. We can definitely make this change so we do start storing all txes on ledger whether successful or failure. Just created a GitHub issue for it: https://github.com/Nuklai/nuklaivm/issues/52

Will make this change before the next reset of our chain. Thanks for elaborating about the use case. I agree it makes sense to store failed transactions on ledger as well.

While the status SUCCESS or FAILED tx will be stored on the ledger, the sdk can handle the PENDING status response as it doesn't make sense to store this on ledger.

kpachhai commented 1 month ago

Has there ever been a case whereby a transaction has failed in your experience?

The reason I ask is because I wanted to take some time to explain how hypervms work.

So, a transaction is created via the SDK and the tx id is immediately returned. However, this doesn't necessarily mean the transaction has even had a time to go through hypervm.

From the hyperchain-js-sdk code, this is what's happening:

      // Sign the transaction
      const [txSigned, err] = tx.sign(authFactory, actionRegistry, authRegistry)
      if (err) {
        return {
          submit: async () => {
            throw new Error('Transaction failed, cannot submit.')
          },
          txSigned: {} as Transaction,
          err: err as Error
        }
      }

      const submit = async (): Promise<SubmitTransactionResponse> => {
        const [txBytes, err] = txSigned.toBytes()
        if (err) {
          throw new Error(`Transaction failed, cannot submit. Err: ${err}`)
        }
        return await this.submitTransaction(txBytes)
      }

      return { submit, txSigned, err: undefined }

So, if the tx was not formed correctly, it would fail immediately.

On nuklai-js-sdk code, we have different functions for different transactions but they all work essentially the same. For a simple transfer transaction,

      const { submit, txSigned, err } =
        await hyperApiService.generateTransaction(
          genesisInfo.genesis,
          actionRegistry,
          authRegistry,
          [transfer],
          authFactory
        )
      if (err) {
        throw err
      }

      await submit()

      return txSigned.id().toString()

For minting asset transaction,

      const { submit, txSigned, err } =
        await hyperApiService.generateTransaction(
          genesisInfo.genesis,
          actionRegistry,
          authRegistry,
          [mintAsset],
          authFactory
        )
      if (err) {
        throw err
      }

      await submit()

      return txSigned.id().toString()

Basically, the submit basically calls the submitTx RPC API of hypersdk if it's a valid transaction. And I took a look at this function and it seems that it basically sends the tx to the mempool and is immediately returned with txID(whether successful or not).

Unfortunately, since this is an RPC API of hypersdk, it's not so easy to change it at the nuklaivm protocol level of how this functionality behaves. Also, regarding showing the PENDING state for a tx, it makes sense to just immediately show any tx as PENDING as soon as it's submitted because the current API of nuklai-js-sdk to retrieve the tx status with getTransactionInfo function interacts with the RPC API which will still not be able to detect any pending transaction UNTIL it's either SUCESSFUL or FAILED.

tldr; With that said, I have put out a change at our nuklaivm protocol level to record the failed transactions so at least if the transaction fails, you can still get the record of it by retrieving info about the tx id from the node. But, this won't go into effect until we reset our blockchain with these new updates(which will likely happen in a few weeks since this same update also includes the updates to the dataset support on our vm).