near / near-workspaces-js

Write tests once, run them both on NEAR TestNet and a controlled NEAR Sandbox local environment
https://near.github.io/near-workspaces-js/
GNU General Public License v3.0
42 stars 22 forks source link

Improve Informativeness of Error Messages: View Calls from User Accounts #185

Closed idea404 closed 2 years ago

idea404 commented 2 years ago

Current error messages can leave the developer guessing what is incorrect in their test module. An example of this is this test case code:

test("Add a hash to the notes of alice", async (t) => {
  const { accounts } = t.context;
  const { contract, alice } = accounts;
  await alice.call(contract, "addNote", {
    ipfsHash: "Qmbx3aPb36KZwrPH1KPT94EsGkyHKVMxf9CaufwQYAkkvX",
  });
  const notes = await alice.view(contract, "getAllNotes", {
    accountId: alice.accountId,
  });
  t.deepEqual(notes, ["Qmbx3aPb36KZwrPH1KPT94EsGkyHKVMxf9CaufwQYAkkvX"]);
});

That when run produced this error:

  Rejected promise returned by test. Reason:

  Error (TypedError) {
    context: undefined,
    type: 'CodeDoesNotExist',
    message: `Querying [object Object] failed: wasm execution failed with error: FunctionCallError(CompilationError(CodeDoesNotExist { account_id: AccountId("alice.test.near") })).␊
    {␊
      "block_hash": "BPQAeGmidJzaRDL7BYsSLCSx8QczB9xNV3TkAqExFCEG",␊
      "block_height": 19,␊
      "error": "wasm execution failed with error: FunctionCallError(CompilationError(CodeDoesNotExist { account_id: AccountId(\\"alice.test.near\\") }))",␊
      "logs": []␊
    }`,
  }

  › }
  › JsonRpcProvider.query (node_modules/near-api-js/lib/providers/json-rpc-provider.js:123:19)
  › Account.view (node_modules/near-workspaces/src/account/account.ts:262:20)
  › async file://__tests__/index.ava.js:39:17

Although the error correctly points out that alice indeed has no code to execute the view command, the error string is a little too ambiguous. Changing the view call in this example to the following allowed the test to execute as intended:

test("Add a hash to the notes of alice", async (t) => {
  const { contract, alice } = t.context.accounts;
  await alice.call(contract, "addNote", {
    ipfsHash: "Qmbx3aPb36KZwrPH1KPT94EsGkyHKVMxf9CaufwQYAkkvX",
  });
  const notes = await contract.view("getAllNotes", {
    accountId: alice.accountId,
  });
  t.deepEqual(notes, ["Qmbx3aPb36KZwrPH1KPT94EsGkyHKVMxf9CaufwQYAkkvX"]);
});

Proposal To implement logic that checks whether this view call can be executed on the account, and if it can not because no wasm file is deployed to it, to return an error message with something along the lines of: "view() call cannot be conducted by account: alice.test.near as no smart contract is deployed to this account"

Link to errorLog: https://gist.github.com/sandoche/9e7decca99b5f590ca9c87d7a225ed08

idea404 commented 2 years ago

Hey @ailisp @volovyks, I was thinking of addressing this issue myself, I am thinking about the design here and was wondering what you would think before I get started on anything. My idea right now is to surround account.callRaw() in account.ts by an error handling class function. Similar to:

  async callRaw(...) {
   ErrorHandlingClass.handle(
    ...
   );
  }

or make us of a decorator. We could implement this interface for the functions within Account for which we'd like to provide more informative error messages. The class would handle the errors by parsing the contents of TransactionResult and returning both the TransactionResult and a richer error message, together as an object perhaps as:

{
  error: "informative string about what actually went wrong",
  result: TransactionResultObject
}

What would you think?

volovyks commented 2 years ago

My understanding is that this error appears mostly because we have two different ways of calling view and call functions.

caller.call(whatContractToCall, 'functionName', { ...params })

and

whatContractView.view('functionName', { ...params })

It make sense if you know what is happening, but new users can be confused.

I do not think that it is necessary to introduce { error, result } pattern, but it is not a strong opinion.

This error originates on the NAJ level (provider). I would rather spend time polishing Error handling on that level first. And only then move to Workspaces. Eventually, Workspaces will be rewritten with the use of updated NAJ.

idea404 commented 2 years ago

I will proceed to close this issue, as the discussion's now been moved to the near-api-js issue board with a proposed approach.