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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Throw by default when awaiting a tx that reverted #5431

Merged
merged 2 commits into from
Mar 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably consider making this behaviour the default :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, dontThrowOnRevert defaults to false, so it throws on reverts by default. Is that what you meant?

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
Loading