feat: Testing with automated crosschain validation framework #10
Conversation
mohammeds1992
changed the title
| expectError: true, | ||
| errorContains: "unauthorized", | ||
| }, | ||
| // BUG 1: Invalid hex characters accepted |
There was a problem hiding this comment.
In this test file, the UpdateAdminParams function is being tested through the message handler.
In Cosmos SDK transactions, there are three main phases after a transaction is created:
- CheckTx – The SDK calls the ValidateBasic() method of each message. Stateless checks are performed here, and the transaction is rejected if any validation fails.
- DeliverTx – Ante handlers are executed first, followed by the message handlers (MsgServer). This phase handles stateful logic.
- Commit – State changes from DeliverTx are committed to the application state.
Because input validation is handled by ValidateBasic() during the CheckTx phase, it does not need to be repeated in the message handler (MsgServer). Therefore, testing input validations in the message handler tests is not appropriate.
Ideally, input validation should be tested in unit tests for the ValidateBasic() method, not in the message handler tests.
| }, | ||
| // BUG 2: Empty factory address accepted | ||
| { | ||
| name: "bug 2: empty factory address accepted", |
| "github.com/rollchains/pchain/x/crosschain/types" | ||
| ) | ||
|
|
||
| func TestUpdateAdminParams(t *testing.T) { |
There was a problem hiding this comment.
tests for msg handlers should not include stateless validation tests
Instead, these should be focused on stateful logic and application behaviour
e.g. correct state transitions, access control, any interaction with other keepers, or any extra functionality that's implemented
| name: "control characters", | ||
| factoryAddr: "0x527F3692F5C53\nCfA83F7689885995606F93b6164", | ||
| expectError: true, | ||
| errorMessage: "contains control characters", |
There was a problem hiding this comment.
In the Cosmos SDK, ValidateBasic() should be cheap and fast, it's intended for lightweight, stateless checks that ensure the message is syntactically valid before any expensive processing happens.
We shouldn't add these specific error messages and validation just for validating a address
a standard "invalid factory address" error msg or a similar one should be fine for address check fails
| errorMessage: "invalid address", | ||
| }, | ||
| { | ||
| name: "zero address", |
There was a problem hiding this comment.
This is an admin controlled field, that's why didn't add a zero address check considering admin is always trusted
But this additional check could be added.
Will add this
| txHash: "0x123", | ||
| expectError: false, // Should this be validated? Worth testing | ||
| }, | ||
| { |
There was a problem hiding this comment.
don't know if all VMs have txHash in a hex format, need to check
| expectError: false, // Should this be validated? Worth testing | ||
| }, | ||
| { | ||
| name: "namespace with special characters", |
There was a problem hiding this comment.
don't think should add that much validations
its supposed to be a cheap stateless validation
and if someone sends invalid characters, it anyway will revert in the msg execution when it'll be used for tx verification
| expectError: false, // Should special chars be allowed in namespace? Worth testing | ||
| }, | ||
| { | ||
| name: "chain id with special characters", |
There was a problem hiding this comment.
shouldn't add this extensive checks
| expectError: false, // Should value limits be validated? Worth testing | ||
| }, | ||
| { | ||
| name: "deadline in the past", |
There was a problem hiding this comment.
doesn't matter, it'd revert in the smart contract
| expectError: false, // Should nonce zero be allowed? Worth testing | ||
| }, | ||
| { | ||
| name: "maxPriorityFeePerGas higher than maxFeePerGas", |
There was a problem hiding this comment.
again, should only check if payload parameters are valid
like target is a valid evm address
string fields are not empty, etc.
rest if someone sends invalid values in parameters, those will error out automatically
No description provided.