Skip to content

fix: include validation reason in PXE simulate error #20768

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.

Sign up for GitHub

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

Merged
nchamo merged 2 commits into from
Feb 24, 2026
Merged

fix: include validation reason in PXE simulate error #20768

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a35c447
fix: include validation reason in PXE simulate error
nchamo Feb 23, 2026
5e5f5b3
refactor: move PXE simulation test to dedicated e2e_pxe test file
nchamo Feb 24, 2026
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
16 changes: 16 additions & 0 deletions yarn-project/end-to-end/src/e2e_expiration_timestamp.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,14 @@ describe('e2e_expiration_timestamp', () => {
.send({ from: defaultAccountAddress }),
).rejects.toThrow(TX_ERROR_INVALID_EXPIRATION_TIMESTAMP);
});

it('simulation includes validation reason in error', async () => {

@benesjan benesjan Feb 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't seem like the best test suite for it as it's not testing the expiration timestamp but pxe.

AI recommended this:

Image

Not sure if e2e_avm is the correct place though as it might be a big pile of ancient mess and this is testing pxe?

@nchamo nchamo Feb 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I initially added the tests here because the original issue reported a problem with discarded error messages on timestamps failures. I would personally leave one of the tests here, so we test the timestamps failures both send and simulate workflows

I did look for other e2e tests to test this better, but I failed to find one that simply tests PXE's handling of errors, or something similar. Would you like me to create one?

@benesjan benesjan Feb 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would just create e2e_pxe test for it and document there what we are testing and add there a todo to ideally eventually make it a non-e2e test.

I would personally leave one of the tests here, so we test the timestamps failures both send and simulate workflows

The problem with this is that it makes the test suite hard to read because nobody would expect that you test pxe here.

await expect(
contract.methods
.set_expiration_timestamp(expirationTimestamp, enqueuePublicCall)
.simulate({ from: defaultAccountAddress }),
).rejects.toThrow(TX_ERROR_INVALID_EXPIRATION_TIMESTAMP);
});
});

describe('with an enqueued public call', () => {
Expand All @@ -135,6 +143,14 @@ describe('e2e_expiration_timestamp', () => {
.send({ from: defaultAccountAddress }),
).rejects.toThrow(TX_ERROR_INVALID_EXPIRATION_TIMESTAMP);
});

it('simulation includes validation reason in error', async () => {

@benesjan benesjan Feb 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above

BTW did you check if it really makes sense to have this be checked by 2 tests? Shouldn't the timestamp expiration be enforced by one code path irrespective of enqueued public calls?

@nchamo nchamo Feb 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It was just to mimic send's behavior, which is tested in both places. But we can keep only one, since it isn't affected by presence/lack of enqueued calls

await expect(
contract.methods
.set_expiration_timestamp(expirationTimestamp, enqueuePublicCall)
.simulate({ from: defaultAccountAddress }),
).rejects.toThrow(TX_ERROR_INVALID_EXPIRATION_TIMESTAMP);
});
});
});

Expand Down
3 changes: 2 additions & 1 deletion yarn-project/pxe/src/pxe.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,7 +958,8 @@ export class PXE {
const validationResult = await this.node.isValidTx(simulatedTx, { isSimulation: true, skipFeeEnforcement });
validationTime = validationTimer.ms();
if (validationResult.result === 'invalid') {
throw new Error('The simulated transaction is unable to be added to state and is invalid.');
const reason = validationResult.reason.length > 0 ? ` Reason: ${validationResult.reason.join(', ')}` : '';
throw new Error(`The simulated transaction is unable to be added to state and is invalid.${reason}`);
}
}

Expand Down