feat: allow parsing Bitcoin deposit memo with inscription #2957
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 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request encompasses a range of updates to the ZetaChain project, including new features, refactoring, tests, fixes, and CI improvements. Key additions are support for stateful precompiled contracts, a Changes
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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2957 +/- ##
========================================
Coverage 66.39% 66.40%
========================================
Files 389 389
Lines 21758 21766 +8
========================================
+ Hits 14447 14454 +7
- Misses 6584 6585 +1
Partials 727 727
|
lumtis
added
no-changelog
lumtis
requested review from
brewmaster012,
kingpinXD,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
🧹 Outside diff range and nitpick comments (12)
contrib/localnet/bitcoin-sidecar/Dockerfile (1)
9-14: Approve second stage with minor enhancement suggestion.The second stage of the Dockerfile is well-structured, utilizing a lightweight Alpine image and correctly copying only the necessary files from the builder stage. The use of the array syntax in the CMD instruction is commendable.
To further optimize the image, consider the following enhancement:
Add a non-root user for running the application:
FROM node:alpine +RUN addgroup -S appgroup && adduser -S appuser -G appgroup +USER appuser COPY --from=builder /home/zeta/node/dist ./dist COPY --from=builder /home/zeta/node/node_modules ./node_modules CMD ["node", "dist/index.js"]This change enhances security by adhering to the principle of least privilege.
e2e/e2etests/test_extract_bitcoin_inscription_memo.go (3)
14-20: Function signature and initial setup look appropriate.The function signature and initial setup are well-structured for an end-to-end test. The conditional mining of blocks adds flexibility for different test environments.
Consider adding error handling for the
SetBtcAddresscall:err := r.SetBtcAddress(r.Name, false) require.NoError(r, err)
33-40: Transaction inscription and block generation are well-implemented.The process of inscribing a transaction, generating blocks, and retrieving transaction details is logically structured with appropriate error handling.
For consistency in logging, consider using a formatted string in the Info log:
r.Logger.Info().Msgf("obtained reveal txn id %s", txid)
53-57: Assertions are correct but could be more informative.The final assertions correctly validate the number of events and the memo content. However, the equality check could benefit from a more descriptive error message.
Enhance the final assertion with a descriptive error message:
require.Equal(r, memo, event.MemoBytes, "Extracted memo does not match the original inscription memo")This change will provide more context in case of a test failure, aiding in debugging and understanding the nature of any potential issues.
contrib/localnet/bitcoin-sidecar/js/src/script.ts (1)
23-25: Clarify the data length validation and error messageThe
pushDatamethod throws an error ifdata.length <= 80, which may exclude valid data lengths. If this restriction is intentional, consider providing a more informative error message explaining why data of length less than or equal to 80 bytes is not supported.Apply this diff to improve the error message:
if (data.length <= 80) { - throw new Error("data length should be more than 80 bytes"); + throw new Error("Data length must exceed 80 bytes for this operation"); }contrib/localnet/bitcoin-sidecar/js/src/index.ts (1)
31-33: Consider adding error handling for server startupAdding error handling for the server startup process can help catch issues such as the port being in use or insufficient permissions.
Proposed Fix:
app.listen(PORT, () => { console.log(`Server is running on http://localhost:${PORT}`); +}).on('error', (err) => { + console.error('Failed to start server:', err); +});e2e/runner/bitcoin_inscription.go (2)
70-70: Remove debugfmt.Printstatement from production code.The
fmt.Printstatement on line 70 appears to be for debugging purposes. Including such statements in production code can clutter output and potentially expose sensitive information.Apply this diff to remove the debug print:
- fmt.Print("raw commit response ", response.Address)
105-116: Simplify response decoding by usingjson.Decoderdirectly.In
GenerateRevealTxn, you read the entire response body into a byte slice before unmarshalling:body, err := io.ReadAll(resp.Body) if err != nil { return "", errors.Wrap(err, "cannot read reveal response body") } // Parse the JSON response var response revealResponse if err := json.Unmarshal(body, &response); err != nil { return "", errors.Wrap(err, "cannot parse reveal response body") }You can streamline this by decoding the JSON response directly from the response body using
json.NewDecoder:var response revealResponse -if err := json.Unmarshal(body, &response); err != nil { - return "", errors.Wrap(err, "cannot parse reveal response body") +if err := json.NewDecoder(resp.Body).Decode(&response); err != nil { + return "", fmt.Errorf("cannot parse reveal response body: %w", err) }This approach is more efficient and consistent with
GenerateCommitAddress.contrib/localnet/bitcoin-sidecar/js/src/client.ts (2)
121-121: Remove redundant semicolonThere's an unnecessary semicolon at the end of the statement, which can be removed to improve code clarity.
Apply this diff to remove the extra semicolon:
-this.psbt = new Psbt({ network });; +this.psbt = new Psbt({ network });
176-176: Correct typo in commentFix the typo in the comment for better readability.
-// should have a way to avoid signing but just providing mocked signautre +// Should have a way to avoid signing by providing a mocked signaturee2e/runner/bitcoin.go (1)
307-308: Address the TODO by replacing the builder with a Go function for improved maintainabilityThe TODO comment indicates the intent to replace the
InscriptionBuilderwith a native Go function to enable instructions, as referenced in issue #2759. Implementing this change will enhance maintainability and allow for more fine-grained control over the inscription creation process.Would you like assistance in implementing this Go function, or should we open a GitHub issue to track this task?
zetaclient/chains/bitcoin/observer/inbound.go (1)
454-454: Address the TODO: SimplifyGetBtcEventWithoutWitnessfunctionThe TODO comment indicates that the
GetBtcEventWithoutWitnessfunction requires simplification. Refactoring this function can enhance its readability and maintainability by reducing complexity and improving the logical flow.Would you like assistance in refactoring this function? I can help propose a cleaner implementation or open a GitHub issue to track this task.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (15)
- changelog.md (1 hunks)
- contrib/localnet/bitcoin-sidecar/Dockerfile (1 hunks)
- contrib/localnet/bitcoin-sidecar/js/package.json (1 hunks)
- contrib/localnet/bitcoin-sidecar/js/src/client.ts (1 hunks)
- contrib/localnet/bitcoin-sidecar/js/src/index.ts (1 hunks)
- contrib/localnet/bitcoin-sidecar/js/src/script.ts (1 hunks)
- contrib/localnet/bitcoin-sidecar/js/src/tsconfig.json (1 hunks)
- contrib/localnet/bitcoin-sidecar/js/src/util.ts (1 hunks)
- contrib/localnet/docker-compose.yml (1 hunks)
- e2e/e2etests/e2etests.go (2 hunks)
- e2e/e2etests/test_extract_bitcoin_inscription_memo.go (1 hunks)
- e2e/runner/bitcoin.go (3 hunks)
- e2e/runner/bitcoin_inscription.go (1 hunks)
- zetaclient/chains/bitcoin/observer/inbound.go (1 hunks)
- zetaclient/chains/bitcoin/observer/inbound_test.go (16 hunks)
✅ Files skipped from review due to trivial changes (2)
- contrib/localnet/bitcoin-sidecar/js/package.json
- contrib/localnet/bitcoin-sidecar/js/src/tsconfig.json
🧰 Additional context used
📓 Path-based instructions (6)
e2e/e2etests/e2etests.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_extract_bitcoin_inscription_memo.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/bitcoin.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/bitcoin_inscription.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/observer/inbound.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/observer/inbound_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
📓 Learnings (1)
zetaclient/chains/bitcoin/observer/inbound.go (1)
Learnt from: ws4charlie PR: zeta-chain/node#2899 File: zetaclient/chains/bitcoin/observer/inbound.go:131-132 Timestamp: 2024-09-19T18:25:57.534Z Learning: ObserveInbound coverage will be improved in future refactor.
🪛 Biome
contrib/localnet/bitcoin-sidecar/js/src/client.ts
[error] 17-17: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead(lint/complexity/noBannedTypes)
[error] 18-18: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead(lint/complexity/noBannedTypes)
[error] 21-21: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead(lint/complexity/noBannedTypes)
🪛 GitHub Check: codecov/patch
zetaclient/chains/bitcoin/observer/inbound.go
[warning] 448-448: zetaclient/chains/bitcoin/observer/inbound.go#L448
Added line #L448 was not covered by tests
🔇 Additional comments (8)
contrib/localnet/docker-compose.yml (1)
230-241: Enhancebitcoin-node-sidecarservice configurationThe
bitcoin-node-sidecarservice has been successfully integrated into the Docker Compose configuration. However, there are several improvements that could enhance its robustness and integration:
Add a dependency on the
bitcoinservice to ensure proper startup order:depends_on: - bitcoinImplement a health check to facilitate orchestration:
healthcheck: test: ["CMD", "curl", "-f", "http://localhost:8000/health"] interval: 30s timeout: 10s retries: 3The
PORTenvironment variable is redundant as it matches the exposed port. Consider removing it unless it's explicitly required by the application.If the service requires persistent data, consider adding a named volume:
volumes: - bitcoin-sidecar-data:/app/dataRemember to define the volume at the bottom of the file:
volumes: bitcoin-sidecar-data:These enhancements will improve the service's integration, reliability, and data persistence within the local development environment.
e2e/e2etests/e2etests.go (2)
85-85: New constant added for Bitcoin inscription memo test.The addition of
TestExtractBitcoinInscriptionMemoNameconstant is appropriate and follows the existing naming convention for test cases.
455-461: New end-to-end test case added for extracting Bitcoin inscription memo.The integration of the new test case
TestExtractBitcoinInscriptionMemois well-structured and consistent with the existing test definitions. It includes a descriptive name, a clear description, and appropriate default arguments.However, to enhance maintainability and consistency, consider the following suggestions:
- Add a comment above the test case to briefly explain its purpose and expected behavior.
- Ensure that the
TestExtractBitcoinInscriptionMemofunction is implemented in the corresponding test file.Consider adding a comment above the test case for better documentation:
+// TestExtractBitcoinInscriptionMemo tests the extraction of memos from Bitcoin inscriptions. +// This test case verifies the system's ability to parse and process inscription data correctly. runner.NewE2ETest( TestExtractBitcoinInscriptionMemoName, "extract memo from BTC inscription", []runner.ArgDefinition{ {Description: "amount in btc", DefaultValue: "0.1"}, }, TestExtractBitcoinInscriptionMemo, ),To ensure the
TestExtractBitcoinInscriptionMemofunction is implemented, run the following command:changelog.md (5)
Line range hint
19-38: LGTM: Important fixes and improvements.This changelog for version v12.2.4 includes several critical fixes that enhance the reliability and performance of the system. Key improvements include:
- Additional checks for external chain height validation.
- Adjustments to gas price calculations for EIP1559.
- Changes to WhitelistERC20 authorization.
- Improvements to Bitcoin transaction handling and fee estimation.
These changes address important issues and should contribute to a more stable and efficient system.
Line range hint
97-219: Major version update with significant breaking changes.Version v12.0.0 introduces substantial changes to the system architecture and functionality:
Breaking Changes:
- Relocation of TSS and chain validation queries from
crosschaintoobservermodule.- Unification of observer sets across all chains.
- Merging of observer params and core params into chain params.
- Changes to the TSS address retrieval for Bitcoin.
New Features:
- Support for Bitcoin inscriptions.
- Addition of state variable to track aborted zeta amount.
- New
snapshotscommands.Important Fixes and Refactoring:
- Improvements to outbound transaction handling and confirmation.
- Refactoring of CCTX scheduler and TSS-related components.
- Updates to various queries for better performance and pagination.
Given the extent of these changes, especially the breaking changes:
A comprehensive migration guide should be provided for users upgrading from previous versions. This guide should detail:
- Changes required in API calls due to relocated and renamed queries.
- How to adapt to the new unified observer set structure.
- Updates needed for systems relying on the old params structure.
Thorough testing is crucial, particularly for:
- The new Bitcoin inscription support.
- The refactored CCTX scheduler.
- The modified TSS address retrieval process for Bitcoin.
Please run the following verification steps:
#!/bin/bash # Verify critical components affected by breaking changes # Test new observer module queries go test -v ./x/observer/... -run "TestGetChainParams|TestObserverSet" # Verify Bitcoin inscription support go test -v ./x/btc/... -run "TestBitcoinInscription" # Check CCTX scheduler refactoring go test -v ./x/crosschain/... -run "TestCCTXScheduler" # Test TSS address retrieval for Bitcoin go test -v ./x/observer/... -run "TestGetTssAddressForBitcoin" echo "Ensure all tests pass and cover the new functionality and breaking changes"
Update all documentation, especially API references, to reflect these changes.
Consider providing a temporary compatibility layer or deprecation warnings for critical changed functionalities to ease the transition for users.
Line range hint
221-253: Approved: Important security enhancements and operational improvements.Version v11.0.0 introduces valuable features and fixes:
Security Enhancements:
- Addition of HSM (Hardware Security Module) capability for zetaclient hot key, which significantly improves key management security.
Operational Improvements:
- New thread for monitoring zeta supply across all connected chains in real-time.
- New transaction type for updating observers, improving flexibility in managing the observer set.
Important Fixes:
- Improvements to deposit handling for paused ZRC20 tokens.
- Enhanced outbound transaction processing for both EVM chains and Bitcoin.
These changes contribute to a more secure and robust system. However, to ensure the reliability of these new features:
Please conduct thorough testing, particularly focusing on:
- HSM integration:
#!/bin/bash # Verify HSM functionality go test -v ./zetaclient/... -run "TestHSMKeyManagement"
- Zeta supply monitoring:
#!/bin/bash # Test zeta supply monitoring thread go test -v ./zetaclient/... -run "TestZetaSupplyMonitoring"
- Observer update mechanism:
#!/bin/bash # Verify observer update functionality go test -v ./x/observer/... -run "TestUpdateObserver"Ensure all tests pass and cover various scenarios, including edge cases.
Line range hint
255-366: Comprehensive improvements across multiple versions.Versions v10.1.2 and earlier introduce a wide range of enhancements:
Key Features:
- External stress testing capabilities (v10.1.2)
- Liquidity cap settings for ZRC20 (v10.1.2)
- Bitcoin block header and merkle proof functionality (v10.1.2)
- TSS funds migration capability (v10.1.2)
Important Fixes:
- Improvements to gas handling and stability pool
- Enhanced chain interactions and authorization checks
- Fixes for various issues in transaction processing and validation
Refactoring and Chores:
- Significant refactoring of core components
- Updates to CI processes and build configurations
Given the extensive changes across these versions:
A comprehensive testing strategy is crucial. Please implement the following:
- Regression Testing:
#!/bin/bash # Run all existing tests to ensure no regressions go test ./...
- Stress Testing:
#!/bin/bash # Utilize the new external stress testing capabilities ./zetacored test stress --duration=1h --transactions=1000
- Liquidity Cap Verification:
#!/bin/bash # Test ZRC20 liquidity cap functionality go test -v ./x/fungible/... -run "TestLiquidityCap"
- Bitcoin Functionality:
#!/bin/bash # Verify Bitcoin block header and merkle proof go test -v ./x/bitcoin/... -run "TestBlockHeaderAndMerkleProof"
- TSS Migration:
#!/bin/bash # Test TSS funds migration process go test -v ./x/tss/... -run "TestFundsMigration"
- Gas Handling:
#!/bin/bash # Verify gas stability pool and related fixes go test -v ./x/gas/... -run "TestGasStabilityPool"Ensure all tests pass and cover the full range of new features and fixes. Consider running extended tests in a staging environment to catch any potential issues that might arise from the interaction of these numerous changes.
Line range hint
40-95: Significant updates and new features introduced.Version v12.1.0 brings substantial changes to the system:
New features:
- Modified emission distribution using fixed block rewards.
- Support for lower gas limits in voting on inbound and outbound transactions.
Important fixes:
- Addressed issues with ballot voting, chain parameter handling, and Bitcoin deposits.
- Improved handling of system transactions and gas fee deductions.
Refactoring:
- Significant reorganization of the zetaclient into subpackages.
- Optimization of return statements and code simplification.
Given the extent of these changes, especially the new features and major refactoring:
It's crucial to ensure comprehensive testing has been performed. Please confirm:
- The new emission distribution mechanism has been thoroughly tested for correctness and performance impact.
- The lower gas limit feature for voting has been verified to not introduce any security vulnerabilities.
- All refactored components, especially in the zetaclient, have been tested for regression issues.
Description
Replace #2727
Include 346c305#diff-3cf4d08d13ce73f11d3f9e415f8060d87b9098bc3837655c6b13fef77078d382R48 changes to disable on mainnet
Summary by CodeRabbit
Release Notes
New Features
/commitand/revealendpoints.Bug Fixes
Tests
Documentation