test: expand apphash test with all state machine msgs #3606
Conversation
ninabarbakadze
changed the title
ninabarbakadze
requested review from
ramin and
evan-forbes
and removed request for
a team
WalkthroughWalkthroughThe modifications enhance the Changes
Sequence Diagram(s)No sequence diagrams provided as the changes are mostly related to testing and setup rather than a new feature or modifications to control flow. Assessment against linked issues
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (3)
Additional comments not posted (10)
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
app/test/consistent_apphash_test.go (1)
47-53: Add a comment explaining the expectedDataRoot.The
expectedDataRootis hardcoded without an explanation. Adding a comment would provide clarity on its origin.+ // Expected data root produced by v1.x - https://github.com/celestiaorg/celestia-app/blob/v1.x/app/consistent_apphash_test.go expectedDataRoot := []byte{100, 59, 112, 241, 238, 49, 50, 64, 105, 90, 209, 211, 49, 254, 211, 83, 133, 88, 5, 89, 221, 116, 141, 72, 33, 110, 16, 78, 5, 48, 118, 72}
| // Expected app hash produced by v1.x - https://github.com/celestiaorg/celestia-app/blob/v1.x/app/consistent_apphash_test.go | ||
| expectedAppHash := []byte{84, 216, 210, 48, 113, 204, 234, 21, 150, 236, 97, 87, 242, 184, 45, 248, 116, 127, 49, 88, 134, 197, 202, 125, 44, 210, 67, 144, 107, 51, 145, 65} | ||
| expectedDataRoot := []byte{100, 59, 112, 241, 238, 49, 50, 64, 105, 90, 209, 211, 49, 254, 211, 83, 133, 88, 5, 89, 221, 116, 141, 72, 33, 110, 16, 78, 5, 48, 118, 72} |
There was a problem hiding this comment.
Consider making expectedAppHash and expectedDataRoot configurable.
Hardcoding values like expectedAppHash and expectedDataRoot might make future updates cumbersome. Consider externalizing these values or making them configurable.
- expectedAppHash := []byte{84, 216, 210, 48, 113, 204, 234, 21, 150, 236, 97, 87, 242, 184, 45, 248, 116, 127, 49, 88, 134, 197, 202, 125, 44, 210, 67, 144, 107, 51, 145, 65}
- expectedDataRoot := []byte{100, 59, 112, 241, 238, 49, 50, 64, 105, 90, 209, 211, 49, 254, 211, 83, 133, 88, 5, 89, 221, 116, 141, 72, 33, 110, 16, 78, 5, 48, 118, 72}
+ expectedAppHash := getExpectedAppHash()
+ expectedDataRoot := getExpectedDataRoot()Committable suggestion was skipped due to low confidence.
| twoInt := sdk.NewInt(2) | ||
|
|
||
| // ---------------- First Block ------------ | ||
| var firstBlockSdkMsgs []sdk.Msg |
There was a problem hiding this comment.
[not blocking]
how much harder / less efficient would it be to use table driven tests? This feels like reading the bible
There was a problem hiding this comment.
I had it in similar format before i changed it to this because i didn't think a super long list of messages was more readable
There was a problem hiding this comment.
but if you think it is i'm happy to change it back to the similar format
app/test/consistent_apphash_test.go
Outdated
| } | ||
|
|
||
| // executeTxs executes a set of transactions and returns the data hash and app hash | ||
| func executeTxs(testApp *app.App, rawBlobTx []byte, rawSdkTxs [][]byte, validators []abci.Validator, lastCommitHash []byte) ([]byte, []byte, error) { |
There was a problem hiding this comment.
[non-blocking]
imo, some version of this would be great to add directly to testapp to use elsewhere!
Co-authored-by: Rootul P <[email protected]>
Co-authored-by: Rootul P <[email protected]>
Co-authored-by: Rootul P <[email protected]>
Co-authored-by: Rootul P <[email protected]>
Co-authored-by: Rootul P <[email protected]>
| ) | ||
|
|
||
| // NewMsgExec - executes the revoke authorization message | ||
| msgExec := authz.NewMsgExec(accountAddresses[0], []sdk.Msg{&msgRevoke}) |
There was a problem hiding this comment.
This is framed weird.
User A should grant permission to User B
User B should use MsgExec to perform the action
User A should revoke User B's permission
But here I believe you have User B performing an action of User A's behalf to revoke permission to execute messages to themself. Maybe it's not that problematic, it's like User B is saying no thank you, I don't want your authority
|
|
||
| // Block 2 height | ||
| blockHeight := testApp.LastBlockHeight() + 2 | ||
| // NewMsgCancelUnbondingDelegation - cancels unbonding delegation from validator-1 |
There was a problem hiding this comment.
This cancels the MsgUndelegate right?
| basicAllowance := feegrant.BasicAllowance{ | ||
| SpendLimit: sdk.NewCoins(sdk.NewCoin(app.BondDenom, sdk.NewIntFromUint64(1000))), | ||
| } | ||
| feegrantMsg, err := feegrant.NewMsgGrantAllowance(&basicAllowance, accountAddresses[0], accountAddresses[1]) |
There was a problem hiding this comment.
Do you have a message in here that actually uses the feegrant allowance?
Good idea! Exporting this into an issue. |
ninabarbakadze
dismissed stale reviews from evan-forbes, cmwaters, and rootulp
via
0891a67
celestia-bot
requested review from
a team,
cmwaters and
evan-forbes
and removed request for
a team
Overview
Resolves #3540 & Resolves #3626
Expected AppHash generated from #3665