test(e2e): add a runner for gov proposals in the e2e test#3591
test(e2e): add a runner for gov proposals in the e2e test#3591
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request enhances the end-to-end testing suite by introducing a governance proposals runner. The changes add new JSON proposal files, update orchestration scripts and Docker configurations, and modify the test runner to create and vote on governance proposals at the start and end of tests. Additionally, key management and governance parameter adjustments have been implemented, and documentation has been updated to detail the governance proposal workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant E2E as E2E Test Runner
participant Gov as Gov Proposal Runner
participant Zeta as Zeta TX Server
participant Validator as Validator Keyring
E2E->>Gov: Invoke CreateGovProposals(Start/End)
Gov->>Gov: Read proposal JSON files
Gov->>Zeta: Submit proposal transaction
Zeta-->>Gov: Return proposal ID / confirmation
Gov->>Validator: Iterate through validators and cast votes
Validator-->>Gov: Return vote confirmations
Gov-->>E2E: Report execution status
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
# Conflicts: # e2e/txserver/zeta_tx_server.go
kingpinXD
changed the title
kingpinXD
changed the title
kingpinXD
changed the title
![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
🧹 Nitpick comments (9)
contrib/localnet/scripts/start-zetacored.sh (1)
191-205: Governance Parameters Update in Genesis Configuration
The modifications to the governance parameters—settingvoting_periodto"30s"andexpedited_voting_periodto"10s"—are well aligned with the new E2E testing requirements for faster proposal processing. Please verify that these shorter durations are appropriate for all testing scenarios and won’t inadvertently affect stability in environments that might mimic production behavior.As a nitpick, note that the jq assignments use inconsistent spacing around the equals sign (e.g., line 192 uses no spaces while line 195 does). For clarity and maintainability, consider unifying the style. For example:
- .app_state.gov.params.expedited_voting_period = "10s" | + .app_state.gov.params.expedited_voting_period="10s" |e2e/TESTING_GUIDE.md (1)
94-98: Documentation for governance proposal execution is clearThe added section clearly explains the two options for executing governance proposals during E2E tests, matching the implementation in the code. This will be valuable for developers wanting to leverage this feature.
Note: There's a minor typo in "governace" (line 96) and a missing space after the period in line 97, but these are trivial.
Suggest fixing the minor typos:
-Any governace proposals can be executed in the E2E tests. There are two options for the sequence of the proposal execution. +Any governance proposals can be executed in the E2E tests. There are two options for the sequence of the proposal execution. -1. Start of E2E tests: The proposal json needs to be placed in the `contrib/localnet/orchestrator/proposal_e2e_start` directory.All proposals in this directory will be executed before running the e2e tests +1. Start of E2E tests: The proposal json needs to be placed in the `contrib/localnet/orchestrator/proposal_e2e_start` directory. All proposals in this directory will be executed before running the e2e testscontrib/localnet/orchestrator/start-zetae2e.sh (1)
145-158: Improve error handling and directory existence checks.
Within the loop, an error log is printed if thekeyring-testdirectory is missing on the remote container, but the script continues to execute. Consider explicitly failing early when a container's keyring directory is missing to avoid partial or inconsistent key copy states.for i in 0 1; do ssh root@zetacore$i "test -d /root/.zetacored/keyring-test/" && \ (echo "Source directory found in zetacore$i, copying operator keys" && \ - mkdir -p /root/zetacore$i/ && \ + if ! mkdir -p /root/zetacore$i/; then + echo "Failed to create local directory /root/zetacore$i/" + exit 1 + fi scp -r root@zetacore$i:/root/.zetacored/keyring-test/ /root/zetacore$i/keyring-test/ && \ echo "Files copied successfully from zetacore$i") || \ (echo "Error: keyring-test directory not found in zetacore$i container" && exit 1) zetacored keys rename operator operator$i --home=/root/zetacore$i/ --keyring-backend=test --yes cp -r /root/zetacore$i/keyring-test/* /root/.zetacored/keyring-test/ donee2e/txserver/zeta_tx_server.go (2)
138-151: Externalize hard-coded paths for portability.
When creating thevalidatorsKeyring, consider using an environment variable or config file reference instead of the hard-coded"/root/.zetacored/"path. This helps ensure maintainability across diverse deployment environments.
166-171: Avoid non-pointer receivers if updating shared state is intended.
Returning a copy ofZetaTxServeron keyring update may be confusing if your usage pattern expects changes to be reflected across the original instance. Consider using a pointer receiver if the same instance needs to be updated in-place.e2e/runner/gov_propsoals.go (4)
113-114: Re-evaluate the assumption that the first proposal ID is always 1.Although this might be true on a fresh chain, the comment here can be misleading if the chain already contains proposals. Consider removing or adjusting the comment.
127-128: Adjust the docstring to match actual functionality.The comment states "Vote yes on the proposal from both the validators," but the function iterates over all validator keys. Update the docstring if more than two validators may exist.
127-143: Consider renaming for clarity.Currently, the function name
voteGovProposalsis plural while it handles a single proposal ID at a time. Renaming it tovoteGovProposalor adding support for multiple proposals could align it better with actual usage.
156-186: Validate proposal JSON structure more thoroughly.While this function successfully parses and unmarshals JSON, it might be beneficial to perform business-level validations (e.g., ensuring the proposal has the required fields) before broadcasting. This could prevent partial or invalid proposals from being processed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
changelog.md(1 hunks)cmd/zetae2e/local/local.go(2 hunks)contrib/localnet/orchestrator/Dockerfile.fastbuild(1 hunks)contrib/localnet/orchestrator/proposals_e2e_end/max_gas.json(1 hunks)contrib/localnet/orchestrator/proposals_e2e_start/emissions_params.json(1 hunks)contrib/localnet/orchestrator/start-zetae2e.sh(2 hunks)contrib/localnet/scripts/start-zetacored.sh(1 hunks)e2e/TESTING_GUIDE.md(1 hunks)e2e/runner/gov_propsoals.go(1 hunks)e2e/txserver/zeta_tx_server.go(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- changelog.md
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.sh`: Review the shell scripts, point out issues relati...
**/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.
contrib/localnet/scripts/start-zetacored.shcontrib/localnet/orchestrator/start-zetae2e.sh
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/local/local.goe2e/txserver/zeta_tx_server.goe2e/runner/gov_propsoals.go
🔇 Additional comments (11)
contrib/localnet/orchestrator/Dockerfile.fastbuild (1)
11-12: COPY commands for governance proposal directories added correctlyThe added COPY commands appropriately transfer governance proposal files from their source directories to the Docker image's working directory, enabling the governance proposal execution feature described in the PR objectives.
cmd/zetae2e/local/local.go (2)
211-212: Governance proposal execution at test start implementedThis code correctly implements the start-of-test execution of governance proposals, aligning with the PR objectives. The placement is logical - after funding the emissions pool but before the main test execution.
495-495: Governance proposal execution at test end implementedThis code correctly implements the end-of-test execution of governance proposals, as described in the PR objectives. The placement is appropriate - after all tests have been run and verified, but before the final success message.
contrib/localnet/orchestrator/proposals_e2e_end/max_gas.json (1)
1-26: Example governance proposal is well-structuredThe JSON structure properly follows the expected format for governance proposals. The proposal correctly includes:
- A properly typed consensus parameter update message
- Authority address
- Appropriate block parameters (max_bytes, max_gas)
- Necessary evidence and validator configurations
- Required metadata, deposit, title, and summary fields
This serves as a good example proposal and will be useful for demonstrating the end-of-test proposal execution feature.
contrib/localnet/orchestrator/start-zetae2e.sh (1)
13-14: Ensure intended SSH usage.
Starting an SSH daemon in this container can pose a security risk if not locked down properly. Confirm that SSH access is truly required, and if so, configure secure credentials and limit inbound connections to avoid unauthorized access.e2e/txserver/zeta_tx_server.go (4)
80-88: Struct extension looks good.
AddingvalidatorsKeyringand other fields provides centralized control and usage clarity. Keep in mind concurrency or reentrancy scenarios if multiple goroutines use this store of credentials at once.
156-163: Object construction is consistent.
Combining all required fields at initialization time is clear and consistent. Ensure thatauthorityClientis set later usingSetAuthorityClientif needed before usage.
182-185: Simple getter is fine.
This straightforward accessor cleanly exposes the underlying codec. No issues to report.
223-226: Keyring accessor suffices.
ExposingvalidatorsKeyringthrough this method is clear and maintains encapsulation. If concurrency or dynamic re-initialization is needed, ensure appropriate locks or usage patterns.contrib/localnet/orchestrator/proposals_e2e_start/emissions_params.json (1)
1-20: Validate large numeric fields.
Ensure that the specified block reward amount ("9620949074074074074.074070733466756687") and percentages reflect intended economic parameters. High-precision values can cause unexpected behavior if not handled consistently across the codebase.e2e/runner/gov_propsoals.go (1)
27-31: Ensure consistency of directory paths and documentation.The comments in lines 27-31 mention directory paths under
/contrib/orchestrator/…while the actual code uses/work/proposals_e2e_.... This discrepancy can lead to confusion if contributors rely on the comment instead of the actual directory path.Would you like to clarify one standardized directory path or keep a distinction between these two references?
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
4 participants
Description
The PR adds a runner to easily test gov proposals.
Any governace proposals can be executed in the E2E tests. There are two options for the sequence of the proposal execution.
contrib/localnet/orchestrator/proposal_e2e_startdirectory.All proposals in this directory will be executed before running the e2e testscontrib/localnet/orchestrator/proposal_e2e_enddirectory.All proposals in this directory will be executed after running the e2e testsCloses: #3518
The required proposals can now easily be added into the e2e tests . We might need to decide on which ones to add ,or we could use this on a as needed basis
How Has This Been Tested?
Summary by CodeRabbit
New Features
Documentation
Chores