fix: add recover to InitChainer to diplay informative message when starting a node from block 1 #2925
Conversation
kingpinXD
requested review from
brewmaster012,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
|
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 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a deferred recovery mechanism in the Changes
Assessment against linked issues
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
|
![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 (2)
pkg/constant/constant.go (1)
44-45: Approve the new constant with suggestions for improvement.The addition of
InitChainErrorMessageis a valuable improvement that addresses the issue outlined in the PR objectives. It provides clear guidance to users attempting to sync from block 1.However, consider the following enhancements:
- Shorten the message for improved readability in logs.
- Include a reference to where users can find appropriate snapshots.
Consider refactoring the constant as follows:
InitChainErrorMessage = "Syncing from block 1 is not supported. Please use a snapshot. For more information, visit: <snapshot_url>"Replace
<snapshot_url>with the actual URL where users can find snapshot information.app/app.go (1)
882-882: Capitalize the error message for consistency and professionalism.The error message should start with an uppercase letter to align with standard logging practices.
Apply this diff to correct the log message:
- ctx.Logger().Error("panic occurred during InitGenesis", "error", r) + ctx.Logger().Error("Panic occurred during InitGenesis", "error", r)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/app.go (3 hunks)
- changelog.md (1 hunks)
- pkg/constant/constant.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/app.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/constant/constant.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (8)
changelog.md (5)
Line range hint
1-45: Ensure comprehensive testing for unreleased changes.The unreleased section contains numerous significant changes, including new features, refactors, and fixes. It's crucial to ensure that each of these changes, especially those affecting core functionality like TSS address handling and chain parameters, undergoes thorough testing before release.
Consider enhancing the changelog entries for more impactful changes. For instance, the changes to TSS address handling and the merging of observer params into chain params could benefit from brief explanations of their implications.
To verify the testing coverage, please run the following command:
#!/bin/bash # Check test coverage for recently modified files git diff --name-only HEAD~10 | grep '\.go$' | xargs -I {} sh -c 'go test -coverprofile=/tmp/cover.out ./$(dirname {}) && go tool cover -func=/tmp/cover.out'
Line range hint
85-105: Assess refactoring impacts and ensure comprehensive testing.The refactoring changes implemented in this release represent significant structural modifications to the codebase. Key points to consider:
- The movement of TSS state and related functionalities from the crosschain module to the observer module may have far-reaching effects on system behavior and API usage.
- Changes to query endpoints and parameter structures (e.g., merging observer params into chain params) will likely require updates to any external systems or scripts interacting with these APIs.
- The reorganization of smoke tests and the addition of pagination to certain queries could affect test coverage and system performance under load.
To mitigate risks associated with these changes:
- Conduct comprehensive integration testing to ensure that all system components interact correctly after the refactoring.
- Update all relevant documentation, especially API references, to reflect the new structure and endpoints.
- Consider creating a migration guide for users of the affected APIs to ease the transition.
To verify the integrity of the refactored code, run integration tests and check for any broken dependencies:
#!/bin/bash # Run integration tests go test ./... -tags=integration # Check for broken imports or unused code go vet ./... golangci-lint run
Line range hint
47-56: Document feature impacts and conduct performance testing.The new features, particularly the addition of monitoring capabilities and dynamic gas pricing, represent significant enhancements to the system. To ensure smooth adoption:
- Consider documenting the expected impact of each feature, especially the dynamic gas pricing, on system performance and user experience.
- For the dynamic gas pricing feature, it's advisable to conduct thorough performance testing to understand its effects on transaction throughput and cost under various network conditions.
Additionally, for the new
snapshotscommands, ensure that comprehensive usage documentation is provided to facilitate user adoption.To verify the performance impact of dynamic gas pricing, consider running the following benchmark:
#!/bin/bash # Run benchmark tests for gas pricing go test -bench=GasPricing ./... -benchmem -benchtime=10s
Line range hint
107-116: Document CI/CD changes and assess their impact.The changes to the CI/CD process, particularly the removal of private runners and unused GitHub Actions, could have significant implications for the project's development workflow:
Removal of private runners:
- Document the rationale behind this decision, addressing any potential impacts on build times, security, or resource allocation.
- Ensure that all team members are aware of how this change might affect their development and testing processes.
Unused GitHub Action removal:
- Confirm that the removed action was indeed unused and that its removal doesn't inadvertently break any dependent processes.
- Update any documentation or team guidelines that may have referenced the removed action.
To maintain the integrity of the CI/CD pipeline:
- Conduct a thorough review of the entire CI/CD process to ensure it remains robust after these changes.
- Consider implementing monitoring for CI/CD performance to detect any negative impacts from the removal of private runners.
To verify the integrity of the CI/CD pipeline after these changes, run:
#!/bin/bash # List all current GitHub Actions workflows gh workflow list # Show recent workflow runs to check for any failures gh run list --limit 10
Line range hint
58-83: Emphasize critical fixes and conduct security audit.The fixes implemented in this release address several critical issues that could significantly impact system stability, security, and correctness. Particularly noteworthy are:
- The fixes for transaction handling, including improvements to outbound tx confirmation and inbound tx verification.
- Security enhancements like masking the zetaclient config at startup.
- Corrections to core functionalities such as TSS address handling and chain parameter management.
Given the security-sensitive nature of many of these changes, it is strongly recommended to:
- Conduct a comprehensive security audit, focusing on the changes related to transaction handling, config masking, and TSS address management.
- Perform thorough regression testing to ensure that these fixes do not inadvertently introduce new issues.
To assist with security verification, consider running a static analysis tool:
app/app.go (3)
7-7: Necessary import of 'runtime/debug' for capturing stack traces.The inclusion of
"runtime/debug"is essential for retrieving stack traces during panic recovery, which enhances error logging and debugging capabilities.
95-95: Importing 'zetaconstant' package to provide informative error messages.Importing
zetaconstantallows access to predefined constants likeInitChainErrorMessage, improving code maintainability and consistency in error messaging.
880-888: Effective panic recovery in 'InitChainer' enhances initialization error handling.The deferred function appropriately handles panics during
InitGenesis, logs the error and stack trace, and provides users with a clear, informative message. This aligns with the objective to improve user experience when starting a node from block 1.
Yeah , I did not face the error mentioned |
Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]>
…arting a node from block 1 (#2925) * add recover to InitChainer * generate files * add docs link to error message * move InitChainErrorMessage to app.go * Update app/app.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * use const for message --------- Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]>
…guration (#2953) * update artillery config * more fixes * feat: integrate authenticated calls smart contract functionality into protocol (#2904) * e2e tests and modifications for authenticated call * extend test with sender check and revert case * separate tests into separate files * cleanup * withdraw and call support and tests * bump protocol contracts * split tests into separate files * small cleanup * fmt * generate * lint * changelog * PR comments * fix case in proto * bump vote inbound gas limit in zetaclient * fix test * generate * fixing tests * call options non empty * generate * test fix * rename gateway caller * pr comments rename tests * PR comment * generate * tests * update tests fixes * tests fixes * fix * test fix * feat!: bank precompile (#2860) * feat: bank precompile * feat: add deposit * feat: extend deposit * PoC: spend amount on behalf of EOA * feat: expand deposit with transferFrom * use CallEVM instead on ZRC20 bindings * divide the contract into different files * initialize e2e testing * remove duplicated funding * add codecov * expand e2e * fix: wait for deposit tx to be mined * apply first round of reviews * cover al error types test * fixes using time.Since * Include CallContract interface * fix eth events in deposit precompile method * emit Deposit event * add withdraw function * finalize withdraw * pack event arguments generically * add high level event function * first round of review fixes * second round of reviews * create bank account when instantiating bank * e2e: add good and bad scenarios * modify fmt * chore: group input into eventData struct * docs: document bank's methods * chore: generate files with suffix .gen.go * chore: assert errors with errorIs * chore: reset e2e test by resetting allowance * test: add first batch of unit test * test: cover all cases * test: complete unit test cases * include review suggestions * include e2e through contract * test: add e2e through contract complete * test: revert balance between tests * Update precompiles/bank/const.go Co-authored-by: Lucas Bertrand <[email protected]> * fix: changed coin denom --------- Co-authored-by: skosito <[email protected]> Co-authored-by: Lucas Bertrand <[email protected]> * feat: add sender to revert context (#2919) * e2e tests and modifications for authenticated call * extend test with sender check and revert case * separate tests into separate files * cleanup * withdraw and call support and tests * bump protocol contracts * split tests into separate files * small cleanup * fmt * generate * lint * changelog * PR comments * fix case in proto * bump vote inbound gas limit in zetaclient * fix test * generate * fixing tests * call options non empty * generate * test fix * rename gateway caller * pr comments rename tests * PR comment * generate * tests * add sender in test contract * extend e2e tests * generate * changelog * PR comment * generate * update tests fixes * tests fixes * fix * test fix * gas limit fixes * PR comment fix * fix bad merge * ci: add option to enable monitoring stack (#2927) * ci: add option to enable monitoring stack * start prometheus faster * update * ci: add rpcimportable test (#2817) * ci: add rpcimportable test * add ci * fmt * use github.com/btcsuite/btcd/btcutil in pkg/chains * remove app imports types tests * use standalone sdkconfig package * fix policies test * move crosschain keeper tests from types to keeper * never seal config in tests * use zeta-chain/ethermint#126 * add some comments * use merged ethermint hash * show resulting go.mod * ci: Add SARIF upload to GitHub Security Dashboard (#2929) * add semgrep sarif upload to GHAS * added comment to clairfy the usage of the utility script * use ghcr.io instead * add tag to image * bad org name --------- Co-authored-by: jkan2 <[email protected]> * fix: add recover to InitChainer to diplay informative message when starting a node from block 1 (#2925) * add recover to InitChainer * generate files * add docs link to error message * move InitChainErrorMessage to app.go * Update app/app.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * use const for message --------- Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * test: add wait for block to tss migration test (#2931) * add wait for block to tss migration test * add comments * refactor identifiers * rename checkNumberOfTssGenerated to checkNumberOfTSSGenerated * chore: allow full zetaclient config overlay (#2945) * test(e2e): add gateway upgrade in upgrade test (#2932) * add gateway upgrade * change reference * add v2 setup for all tests * test v2 in light upgrade * refactor setup to use custody v2 directly * chore: improve localnet build performance (#2928) * chore: improve localnet build performance * propagate NODE_VERSION and NODE_COMMIT * update hashes --------- Co-authored-by: skosito <[email protected]> Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> Co-authored-by: Lucas Bertrand <[email protected]> Co-authored-by: Alex Gartner <[email protected]> Co-authored-by: jkan2 <[email protected]> Co-authored-by: jkan2 <[email protected]> Co-authored-by: Tanmay <[email protected]>
Description
Closes #2685
How Has This Been Tested?
Summary by CodeRabbit
New Features
Documentation