test: consistent app hash on v2 #3879
Conversation
ninabarbakadze
requested review from
rootulp and
staheri14
and removed request for
a team
WalkthroughWalkthroughThe changes in the pull request enhance the testing framework for application hashes by introducing a structured approach to testing consistency across different versions of the Celestia application. A new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/test/consistent_apphash_test.go (1)
Line range hint
152-323: LGTM with a suggestion!The
encodedSdkMessagesV1function is responsible for creating and encoding various SDK messages for v1 of the application. The function is well-structured, with a clear separation of messages for each block. It covers a wide range of SDK modules and creates messages with deterministic values.The function uses appropriate naming conventions for variables and messages, making it easy to understand the purpose of each message.
However, the function is quite large and contains a lot of message creation logic. To improve readability and maintainability, consider refactoring the message creation logic into separate smaller functions for each module or message type. This will make the code more modular and easier to understand and maintain.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/test/consistent_apphash_test.go (5 hunks)
Additional comments not posted (5)
app/test/consistent_apphash_test.go (5)
62-135: LGTM!The
TestConsistentAppHashfunction is well-structured and comprehensive. It covers testing the consistency of app hashes across different versions of the application. The test cases are defined in a clear and organized manner using theappHashTeststruct. The test execution flow is logical and easy to follow.The test cases cover both v1 and v2 of the application, ensuring that the app hashes remain consistent across different versions. The test also verifies the consistency of the data root.
Overall, the changes enhance the testing framework and provide a robust way to ensure the consistency of app hashes.
137-150: LGTM!The
getAccountsAndCreateSignerfunction is a helper utility that retrieves accounts from the keyring, queries their info, and creates a signer with those accounts. The function is well-named, and its purpose is clear. It takes the necessary parameters and returns the signer and account addresses.The function does not contain any complex logic and serves as a useful utility for setting up the test environment.
325-338: LGTM!The
encodedSdkMessagesV2function is responsible for creating and encoding SDK messages specific to v2 of the application. The function is small and focused, creating only theMsgTryUpgradeandMsgSignalVersionmessages.The function follows a similar structure to
encodedSdkMessagesV1, using theprocessSdkMessagesfunction to create encoded transactions. This consistency makes it easy to understand and maintain.The function is well-named and its purpose is clear.
340-354: LGTM!The
createEncodedBlobTxfunction is responsible for creating, signing, and returning an encoded blob transaction. The function follows a clear and focused logic.It takes the necessary parameters, creates a new blob using a fixed namespace and deterministic data, and constructs a
blobTxstruct with the required fields. It then uses the signer to create aPayForBlobstransaction with the blob and default transaction options.The function uses appropriate naming conventions and structs to organize the data, making it easy to understand and maintain.
Line range hint
420-512: LGTM!The
executeTxsfunction is responsible for executing a set of transactions and returning the data hash and app hash. The function is well-structured and follows a clear sequence of steps.It takes the necessary parameters, including the test app, encoded transactions, validators, last commit hash, and app version. It prepares a proposal, processes it, begins a block, delivers the transactions, ends the block, and commits the state using the appropriate functions of the test app.
The function handles both SDK transactions and blob transactions, performing necessary checks and validations throughout the process. It retrieves the app hash using the
LastCommitIDfunction of the test app and returns the data hash and app hash.The function is well-organized and easy to follow, making it maintainable and understandable.
| name: "execute sdk messages and blob tx on v2", | ||
| version: v2.Version, | ||
| encodedSdkMessages: func(t *testing.T, accountAddresses []sdk.AccAddress, genValidators []stakingtypes.Validator, testApp *app.App, signer *user.Signer, valSigner *user.Signer) ([][]byte, [][]byte, [][]byte) { | ||
| firstBlockEncodedTxs, secondBlockEncodedTxs, thirdBlockEncodedTxs := encodedSdkMessagesV1(t, accountAddresses, genValidators, testApp, signer, valSigner) |
There was a problem hiding this comment.
v2 removed x/blobstream so I think the blobstream transactions need to be removed the test case on v2. Alternatively if they aren't removed, maybe this test case should verify that x/blobstream messages are no-ops on v2.
There was a problem hiding this comment.
I think those messages are supposed to be rejected on v2. I'll investigate.
There was a problem hiding this comment.
It looks like the blobstream messages were never part of the v1 test suite anyway so I think doing it like this is fine
There was a problem hiding this comment.
If the v1 test doesn't have blobstream messages, is it missing any other messages? Do we have a GH issue to make this test exhaustive of all message types supported by each app version?
There was a problem hiding this comment.
it covers most of it now. the only message we don't execute is MsgSubmitEvidence and IBC messages that require a different setup. Initially, we wanted to cover as much as possible and we're now extending to cover IBC messages too.
I'm extending the test to include all IBC messages for all verisons |
celestia-bot
requested review from
a team and
evan-forbes
and removed request for
a team
After looking into adding IBC messages I realised that it would require me to add more test chains, set up relayers, etc. I think it's best if we have a separate test for executing IBC messages and comparing AppHash between versions. Thoughts? @cmwaters @rootulp |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/test/consistent_apphash_test.go (1)
Line range hint
154-353: Approved with suggestionsThe encodedSdkMessagesV1 function generates and returns encoded SDK messages for v1 of the application. The separation of message creation logic into a separate function improves the readability and maintainability of the test code. The division of messages into three blocks helps in organizing the message creation logic.
However, the function is quite large and contains a lot of message creation logic for different scenarios. Consider breaking down the function further into smaller, more focused functions based on the type of messages being created. This will enhance the readability and maintainability of the code even further.
Additionally, consider adding comments to explain the purpose of each block of messages and the scenarios they cover. This will make it easier for other developers to understand the intent behind each block of messages.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/test/consistent_apphash_test.go (5 hunks)
Additional comments not posted (14)
app/test/consistent_apphash_test.go (14)
11-12: LGTM!The imports for v1 and v2 appconsts are necessary to access version-specific constants.
18-19: LGTM!The imports for blobstreamtypes and signal are necessary to access types from the respective modules.
22-29: LGTM!The additional imports from the cosmos-sdk are necessary to access various types and modules used in the test.
38-38: LGTM!The import for gethcommon from go-ethereum is necessary to access the HexToAddress function used in the test.
45-49: LGTM!The blobTx struct is well-defined with necessary fields to represent a blob transaction.
51-54: LGTM!The type aliases encodedSdkMessages and encodedBlobTxs provide a cleaner way to define function signatures for generating encoded SDK messages and blob transactions.
56-63: LGTM!The appHashTest struct is well-defined with necessary fields to represent a test case for the app hash consistency test. The struct provides a clean way to organize test case details.
65-137: LGTM!The TestConsistentAppHash function is well-structured and comprehensive. It covers the necessary steps to verify app hash consistency across different versions of the application. The use of the appHashTest struct provides a clean and maintainable way to define and iterate over test cases. The modularization of test setup, transaction generation, and execution logic enhances the readability and maintainability of the code. The test performs thorough consistency checks by verifying both the app hash and data root against expected values.
139-152: LGTM!The getAccountsAndCreateSigner function encapsulates the logic for retrieving accounts from the keyring and creating a signer with the accounts. This promotes code reuse and improves readability. The function takes necessary parameters and returns the signer and account addresses, which can be used in other parts of the test. The separation of concerns and modularization of the signer creation logic is a good practice.
304-313: LGTM!The code segment demonstrates the creation of a periodic vesting account using the NewMsgCreatePeriodicVestingAccount function. The vesting periods are properly defined using the vestingtypes.Period struct, and the message is appended to the secondBlockSdkMsgs slice to be included in the second block of messages. The usage of the vesting account creation message is correct and follows the expected pattern.
315-318: LGTM!The code segment demonstrates the creation of a permanent locked account using the NewMsgCreatePermanentLockedAccount function. The usage of the function is correct, and the message is properly appended to the secondBlockSdkMsgs slice to be included in the second block of messages. The creation of a permanent locked account follows the expected pattern.
320-323: LGTM!The code segment demonstrates the creation of a vesting account using the NewMsgCreateVestingAccount function. The usage of the function is correct, and the necessary parameters such as vesting amount, vesting start time, and vesting end time are provided. The message is properly appended to the secondBlockSdkMsgs slice to be included in the second block of messages. The creation of a vesting account follows the expected pattern.
338-343: LGTM!The code segment demonstrates the registration of an EVM address using the NewMsgRegisterEVMAddress function. The registration is conditionally performed based on the application version, which is a good practice to ensure compatibility with different versions. The message is properly appended to the thirdBlockSdkMsgs slice to be included in the third block of messages. The usage of the EVM address registration message is correct and follows the expected pattern.
355-368: LGTM!The encodedSdkMessagesV2 function encapsulates the logic for generating v2-specific SDK messages, promoting code reuse and readability. The separation of v2-specific message creation logic into a separate function improves the organization and maintainability of the test code. The function takes necessary parameters such as validators and a validator signer to create the messages. It returns the encoded transactions, which can be used in other parts
|
Don't we already have tests where we send IBC messages (i.e. ICA and Transfer) can't we just add the app hash test vectors within those tests? |
yeah we could. I'll open an issue for it. |
Overview
Fixes #3624