Skip to content

Commit

Permalink
feat: Throw by default when awaiting a tx that reverted (#5431)
Browse files Browse the repository at this point in the history
A tx that returns from a `tx.wait()` is then guaranteed to have
succeeded. Otherwise, we were having txs that failed silently, since we
were not manually checking the receipt status after every action.
  • Loading branch information
spalladino authored Mar 25, 2024
1 parent 239ebfb commit c9113ec
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 9 deletions.
6 changes: 4 additions & 2 deletions yarn-project/aztec.js/src/contract/sent_tx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ export type WaitOpts = {
waitForNotesSync?: boolean;
/** Whether to include information useful for debugging/testing in the receipt. */
debug?: boolean;
/** Whether to accept a revert as a status code for the tx when waiting for it. If false, will throw if the tx reverts. */
dontThrowOnRevert?: boolean;
};

export const DefaultWaitOpts: WaitOpts = {
Expand Down Expand Up @@ -60,10 +62,10 @@ export class SentTx {
*/
public async wait(opts?: WaitOpts): Promise<FieldsOf<TxReceipt>> {
if (opts?.debug && opts.waitForNotesSync === false) {
throw new Error('Cannot set getNotes to true if waitForNotesSync is false');
throw new Error('Cannot set debug to true if waitForNotesSync is false');
}
const receipt = await this.waitForReceipt(opts);
if (receipt.status !== TxStatus.MINED && receipt.status !== TxStatus.REVERTED) {
if (!(receipt.status === TxStatus.MINED || (receipt.status === TxStatus.REVERTED && opts?.dontThrowOnRevert))) {
throw new Error(
`Transaction ${await this.getTxHash()} was ${receipt.status}. Reason: ${receipt.error ?? 'unknown'}`,
);
Expand Down
15 changes: 10 additions & 5 deletions yarn-project/end-to-end/src/e2e_deploy_contract.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,10 @@ describe('e2e_deploy_contract', () => {

await Promise.all([goodDeploy.simulate(firstOpts), badDeploy.simulate(secondOpts)]);
const [goodTx, badTx] = [goodDeploy.send(firstOpts), badDeploy.send(secondOpts)];
const [goodTxPromiseResult, badTxReceiptResult] = await Promise.allSettled([goodTx.wait(), badTx.wait()]);
const [goodTxPromiseResult, badTxReceiptResult] = await Promise.allSettled([
goodTx.wait(),
badTx.wait({ dontThrowOnRevert: true }),
]);

expect(goodTxPromiseResult.status).toBe('fulfilled');
expect(badTxReceiptResult.status).toBe('fulfilled'); // but reverted
Expand Down Expand Up @@ -396,7 +399,7 @@ describe('e2e_deploy_contract', () => {
const receipt = await contract.methods
.increment_public_value(whom, 10)
.send({ skipPublicSimulation: true })
.wait();
.wait({ dontThrowOnRevert: true });
expect(receipt.status).toEqual(TxStatus.REVERTED);

// Meanwhile we check we didn't increment the value
Expand Down Expand Up @@ -436,10 +439,12 @@ describe('e2e_deploy_contract', () => {
}, 60_000);

it('refuses to initialize the instance with wrong args via a public function', async () => {
// TODO(@spalladino): This tx is mined but reverts, we need to check revert flag once it's available
// Meanwhile, we check that its side effects did not come through as a means to assert it reverted
const whom = AztecAddress.random();
await contract.methods.public_constructor(whom, 43).send({ skipPublicSimulation: true }).wait();
const receipt = await contract.methods
.public_constructor(whom, 43)
.send({ skipPublicSimulation: true })
.wait({ dontThrowOnRevert: true });
expect(receipt.status).toEqual(TxStatus.REVERTED);
expect(await contract.methods.get_public_value(whom).view()).toEqual(0n);
}, 30_000);

Expand Down
2 changes: 1 addition & 1 deletion yarn-project/end-to-end/src/e2e_fees.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ describe('e2e_fees', () => {
paymentMethod: new PublicFeePaymentMethod(bananaCoin.address, bananaFPC.address, wallets[0]),
},
})
.wait();
.wait({ dontThrowOnRevert: true });
expect(txReceipt.status).toBe(TxStatus.REVERTED);

// and thus we paid the fee
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/end-to-end/src/guides/dapp_testing.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ describe('guides/dapp/testing', () => {
it('asserts a transaction with a failing public call is included (with no state changes)', async () => {
// docs:start:pub-reverted
const call = token.methods.transfer_public(owner.getAddress(), recipient.getAddress(), 1000n, 0);
const receipt = await call.send({ skipPublicSimulation: true }).wait();
const receipt = await call.send({ skipPublicSimulation: true }).wait({ dontThrowOnRevert: true });
expect(receipt.status).toEqual(TxStatus.REVERTED);
const ownerPublicBalanceSlot = cheats.aztec.computeSlotInMap(6n, owner.getAddress());
const balance = await pxe.getPublicStorageAt(token.address, ownerPublicBalanceSlot);
Expand Down

0 comments on commit c9113ec

Please sign in to comment.