Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix isSuccess always returning true, even if transaction fails #826

Merged
merged 10 commits into from
Apr 24, 2023

Conversation

MartinMinkov
Copy link
Contributor

@MartinMinkov MartinMinkov commented Apr 3, 2023

Description

馃敆 Related Issue: #819

This PR addresses the following improvements:

  • Refactors the GraphQL request response and error-checking logic for better maintainability.
  • Modifies the transaction status-checking process after broadcasting. Instead of using the existing query, we introduce a new bestChain query to examine the failureReason returned from the node's GraphQL interface.

The problem was that the transactionStatus query checks if a transaction has been successfully broadcasted and included in a block. However, when a zkApp transaction fails, it is still technically included in a block but without applying any updates. isSucess checks for initial request errors, and if the transaction is broadcasted, then it's true.

Consequently, the transactionStatus query reports that the transaction has been included, leading to .wait() succeeding and isSuccess status of true since it was broadcasted correctly. Using the bestChain query to verify applied zkApp transactions, we can more accurately determine if AccountUpdates have failed and identify the reason for the failure. If we call .wait(), and a transaction fails, an error is thrown.

Lastly, this PR enhances the error messages for better user experience and understanding.

Testing

This PR was manually tested using the following script to deploy and ensure that the expected transaction (payout 2) fails and isSucess shows false.

import {
  fetchAccount,
  Permissions,
  Field,
  method,
  UInt64,
  PrivateKey,
  SmartContract,
  Mina,
  AccountUpdate,
  isReady,
  Bool,
  PublicKey,
  shutdown,
} from 'snarkyjs';

await isReady;

class SimpleZkapp extends SmartContract {
  events = { update: Field, payout: UInt64, payoutReceiver: PublicKey };

  init() {
    super.init();
    this.account.permissions.set({
      ...Permissions.default(),
      send: Permissions.proof(),
      editState: Permissions.proof(),
    });
  }

  /**
   * This method allows a certain privileged account to claim half of the zkapp balance, but only once
   * @param caller the privileged account
   */
  @method payout(caller: PrivateKey) {
    let callerAddress = caller.toPublicKey();
    callerAddress.assertEquals(privilegedAddress);

    let callerAccountUpdate = AccountUpdate.create(callerAddress);
    callerAccountUpdate.account.isNew.assertEquals(Bool(true));

    let balance = this.account.balance.get();
    this.account.balance.assertEquals(balance);
    let halfBalance = balance.div(2);
    this.send({ to: callerAccountUpdate, amount: halfBalance });

    // emit some events
    this.emitEvent('payoutReceiver', callerAddress);
    this.emitEvent('payout', halfBalance);
  }

  deposit(amount: UInt64) {
    let senderUpdate = AccountUpdate.createSigned(this.sender);
    senderUpdate.send({ to: this, amount });
  }
}

// Test following URLs:
// https://proxy.berkeley.minaexplorer.com/graphql
// https://berkeley.minascan.io/graphql
// Mina GraphQL endpoint

const Berkeley = Mina.Network({
  mina: 'https://proxy.berkeley.minaexplorer.com/graphql',
  archive: 'https://archive-node-api.p42.xyz/',
});
Mina.setActiveInstance(Berkeley);

// a test account that pays all the fees, and puts additional funds into the zkapp
let feePayerKey = PrivateKey.random();
let feePayerAddress = feePayerKey.toPublicKey();
// let feePayerKey = Local.testAccounts[0].privateKey;
// let feePayerAddress = Local.testAccounts[0].publicKey;

// the zkapp account
let zkappKey = PrivateKey.random();
let zkappAddress = zkappKey.toPublicKey();

// a special account that is allowed to pull out half of the zkapp balance, once
let privilegedKey = PrivateKey.random();
let privilegedAddress = privilegedKey.toPublicKey();

let initialBalance = 10_000_000_000;
let zkapp = new SimpleZkapp(zkappAddress);

console.log('waiting for accounts to receive funds...');
// Await all promises.
await Promise.all([Mina.faucet(feePayerAddress), Mina.faucet(zkappAddress)]);
console.log('funds received');

console.log(`using the following addressed:
feePayer: ${feePayerAddress.toBase58()}
privilegedKey contract: ${privilegedKey.toPublicKey().toBase58()}
zkappKey contract: ${zkappKey.toPublicKey().toBase58()}
`);

console.log('compile');
await SimpleZkapp.compile();
let nonce = 0;

console.log('TRANSACTION: deploy');
let tx = await Mina.transaction(
  { sender: feePayerAddress, fee: 100_000_000, memo: 'Deploy', nonce },
  () => {
    zkapp.deploy({ zkappKey });
  }
);
await tx.prove();
let id1 = await tx.sign([feePayerKey, privilegedKey]).send();
await waitAndCheckTransactionStatus(id1);
nonce++;

console.log('TRANSACTION: payout 1\n');
await fetchAccount({ publicKey: zkappAddress });
tx = await Mina.transaction(
  { sender: feePayerAddress, fee: 100_000_000, nonce, memo: 'Payout 1' },
  () => {
    AccountUpdate.fundNewAccount(feePayerAddress);
    zkapp.payout(privilegedKey);
  }
);
await tx.prove();
let id3 = await tx.sign([feePayerKey]).send();
await waitAndCheckTransactionStatus(id3);
nonce++;

console.log('TRANSACTION: payout 2 (expected to fail)\n');
await fetchAccount({ publicKey: zkappAddress });
tx = await Mina.transaction(
  { sender: feePayerAddress, fee: 100_000_000, nonce, memo: 'Payout 2' },
  () => {
    zkapp.payout(privilegedKey);
  }
);

// this tx should fail, but we wont know that here - so we just check that no state has changed
await tx.prove();
let id4 = await tx.sign([feePayerKey]).send();
await waitAndCheckTransactionStatus(id4);

async function waitAndCheckTransactionStatus(txn: Mina.TransactionId) {
  let status = txn.isSuccess;
  console.log('----------------------------------');
  console.log(
    `Before wait: Transaction ${txn.hash()} ${
      status ? 'succeeded' : 'failed'
    } with status ${status}`
  );
  await txn.wait();
  status = txn.isSuccess;
  console.log(
    `After wait: Transaction ${txn.hash()} ${
      status ? 'succeeded' : 'failed'
    } with status ${status}`
  );
  console.log('----------------------------------');
}
await shutdown();

/* Output:
> tsc

waiting for accounts to receive funds...
funds received
using the following addressed:
feePayer: B62qqiSncoJeqKpnNhHBVVJ5qpLHAicEqrZkxojphVNu2oMkR2rrwZ5
privilegedKey contract: B62qrDzY6cfaPXj9jThBJdXotEPTVER5pCDwt7K7PJBr7Qkifhz58A1
zkappKey contract: B62qjwY78AX5qzznyjushnFjFfXW6zoUovyydfGz3mQLuzHZC5ApYmh

compile
TRANSACTION: deploy
----------------------------------
Before wait: Transaction 5Jus8TgLtjBsCF7RywXtmeLswvWymutNSJapbgEGRTbBhraVQ7Yi succeeded with status true
After wait: Transaction 5Jus8TgLtjBsCF7RywXtmeLswvWymutNSJapbgEGRTbBhraVQ7Yi succeeded with status true
----------------------------------
TRANSACTION: payout 1

----------------------------------
Before wait: Transaction 5JvFSkugGAAZV7buHG67AeFobsf1beM7SHc9X37w9TAWQYHAizbS succeeded with status true
After wait: Transaction 5JvFSkugGAAZV7buHG67AeFobsf1beM7SHc9X37w9TAWQYHAizbS succeeded with status true
----------------------------------
TRANSACTION: payout 2 (expected to fail)

----------------------------------
Before wait: Transaction 5JugYijF6bZCtLTbczemSRSGzZ8nwK8oP7eoMoDRj2Uhe9QRrXrJ succeeded with status true
/home/martin/Code/o1/mina/src/lib/snarky_js_bindings/snarkyjs/dist/node/_node_bindings/snarky_js_node.bc.cjs:7512
         throw err;
         ^

Error: Transaction failed.
        TransactionId: 5JugYijF6bZCtLTbczemSRSGzZ8nwK8oP7eoMoDRj2Uhe9QRrXrJ
        Attempts: 18
        failureReason(s): AccountUpdate #1 failed. Reason: "Cancelled", AccountUpdate #2 failed. Reason: "Account_is_new_precondition_unsatisfied"
    at Timeout.executePoll [as _onTimeout] (file:///home/martin/Code/o1/mina/src/lib/snarky_js_bindings/snarkyjs/dist/node/lib/mina.js:507:43)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
*/

@MartinMinkov MartinMinkov linked an issue Apr 3, 2023 that may be closed by this pull request
@nicc nicc added the product-eng For tracking our team's issues label Apr 11, 2023
@nicc
Copy link
Contributor

nicc commented Apr 12, 2023

IRI: @Trivo25

@MartinMinkov MartinMinkov changed the title [WIP] Fix isSuccess always returning true, even if transaction fails Fix isSuccess always returning true, even if transaction fails Apr 21, 2023
@MartinMinkov MartinMinkov marked this pull request as ready for review April 21, 2023 21:03
@MartinMinkov MartinMinkov merged commit 91c2009 into main Apr 24, 2023
7 checks passed
@MartinMinkov MartinMinkov deleted the fix/isSuccess branch April 24, 2023 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product-eng For tracking our team's issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

isSuccess always returns true
3 participants