chore: blk-01 #1794
chore: blk-01 #1794
Conversation
|
Warning Rate limit exceeded@claytonneal has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request implements enhancements across multiple packages. In the core module, the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as RPC Client
participant RPC as RPC Method
participant Converter as DefaultBlockToRevision
participant VeChain as VeChain Client
Client->>RPC: Call method with DefaultBlock parameter
RPC->>Converter: Convert DefaultBlock to Revision
Converter-->>RPC: Return revision value
RPC->>VeChain: Execute call using revision.toString()
VeChain-->>RPC: Return response
RPC-->>Client: Send response back
Suggested labels
Suggested reviewers
Poem
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: 2
🔭 Outside diff range comments (1)
packages/network/src/provider/utils/rpc-mapper/methods/eth_getBlockByNumber/eth_getBlockByNumber.ts (1)
33-36:⚠️ Potential issueUpdate input validation to match new DefaultBlock type.
The input validation still checks for
typeof params[0] !== 'string'but the parameter type has been changed toDefaultBlockwhich could be a number or other types.Apply this diff to fix the validation:
if ( params.length !== 2 || - typeof params[0] !== 'string' || typeof params[1] !== 'boolean' )
🧹 Nitpick comments (2)
packages/network/src/provider/utils/const/blocks/blocks.ts (1)
31-50: Consider making 'pending' an explicit case.
Currently, 'pending' is mapped to'best'in theelseclause alongside 'latest'. While functionally it works, adding a dedicated check for 'pending' could enhance readability and reduce confusion.For example:
} else if (defaultBlock === 'pending') { return Revision.of('best'); } else { return Revision.of('best'); }packages/rpc-proxy/tests/e2e_rpc_proxy.solo.test.ts (1)
295-295: Consider adding test coverage for block hash parameter.While testing with block number '0x0' is valid, consider adding another test case using a block hash to ensure comprehensive coverage of the
eth_getBlockReceiptsmethod.it('eth_getBlockReceipts method call', async () => { const response = await axios.post(RPC_PROXY_URL, { jsonrpc: '2.0', method: 'eth_getBlockReceipts', params: ['0x0'], id: 1 }); + // Test with block hash + const responseWithHash = await axios.post(RPC_PROXY_URL, { + jsonrpc: '2.0', + method: 'eth_getBlockReceipts', + params: ['0x0000000008602e7a995c747a3215b426c0c65709480b9e9ac57ad37c3f7d73de'], + id: 2 + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/core/src/vcdm/Revision.ts(3 hunks)packages/core/tests/vcdm/Revision.unit.test.ts(2 hunks)packages/network/src/provider/utils/const/blocks/blocks.ts(1 hunks)packages/network/src/provider/utils/rpc-mapper/index.ts(0 hunks)packages/network/src/provider/utils/rpc-mapper/methods/eth_call/eth_call.ts(3 hunks)packages/network/src/provider/utils/rpc-mapper/methods/eth_estimateGas/eth_estimateGas.ts(3 hunks)packages/network/src/provider/utils/rpc-mapper/methods/eth_getBalance/eth_getBalance.ts(2 hunks)packages/network/src/provider/utils/rpc-mapper/methods/eth_getBlockByHash/eth_getBlockByHash.ts(3 hunks)packages/network/src/provider/utils/rpc-mapper/methods/eth_getBlockByNumber/eth_getBlockByNumber.ts(3 hunks)packages/network/src/provider/utils/rpc-mapper/methods/eth_getCode/eth_getCode.ts(2 hunks)packages/network/src/provider/utils/rpc-mapper/methods/eth_getStorageAt/eth_getStorageAt.ts(2 hunks)packages/network/src/provider/utils/rpc-mapper/rpc-mapper.ts(1 hunks)packages/network/src/provider/utils/rpc-mapper/types.d.ts(0 hunks)packages/network/tests/provider/rpc-mapper/methods/eth_call/eth_call.mock.solo.test.ts(2 hunks)packages/rpc-proxy/README.md(1 hunks)packages/rpc-proxy/tests/e2e_rpc_proxy.solo.test.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/network/src/provider/utils/rpc-mapper/index.ts
- packages/network/src/provider/utils/rpc-mapper/types.d.ts
🧰 Additional context used
🪛 ESLint
packages/network/src/provider/utils/rpc-mapper/methods/eth_getBlockByHash/eth_getBlockByHash.ts
[error] 10-10: 'DefaultBlock' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 10-10: Remove this unused import of 'DefaultBlock'.
(sonarjs/unused-import)
[error] 10-10: 'DefaultBlockToRevision' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 10-10: Remove this unused import of 'DefaultBlockToRevision'.
(sonarjs/unused-import)
🪛 LanguageTool
packages/rpc-proxy/README.md
[grammar] ~275-~275: A determiner may be missing.
Context: ... | best block | | earliest block | block n...
(THE_SUPERLATIVE)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: rpc-proxy / test / test
- GitHub Check: unit-integration-test-browser / Build & Lint (latest)
- GitHub Check: unit-integration-test / Build & Lint (latest)
- GitHub Check: unit-integration-test-browser / Build & Lint (lts/*)
- GitHub Check: unit-integration-test / Build & Lint (lts/*)
- GitHub Check: unit-integration-test-browser / Build & Lint (18)
- GitHub Check: unit-integration-test / Build & Lint (18)
- GitHub Check: install-build / Build & Lint
- GitHub Check: test-apps / Install and test example apps
- GitHub Check: Execute doc examples
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (26)
packages/network/src/provider/utils/const/blocks/blocks.ts (4)
1-1: Import looks good.
The import statement is straightforward and properly references theHexUIntandRevisionmodules. No issues identified here.
3-16: Type definition for DefaultBlock is clear.
DefiningDefaultBlockas a union of known tags and a hex string appropriately models Ethereum block references. ThedefaultBlockTagsarray helps validate permissible tags. This is concise and correct.
19-29: High-quality documentation.
Your docstring, explaining the mapping from Ethereum default blocks to VeChainThor revisions, is thorough and complements the code. Users will appreciate the clarity.
52-52: Export statement is consistent.
Exporting bothDefaultBlockandDefaultBlockToRevisiontogether is logical and improves discoverability for these related utilities.packages/core/src/vcdm/Revision.ts (7)
6-13: Appreciate the expanded revision documentation.
The docstrings now cover all recognized revision keywords and formats, offering clarity on valid inputs.
23-24: Regex approach is straightforward and comprehensive.
Your new pattern accommodates all valid revision forms. The grouping of keywords, hex, and digits under a single regex is an efficient solution.
27-28: Documentation for extended value types.
Stating thatbigintandHexare acceptable types ensures that developers understand the broader scope of valid input.
31-41: RobustisValidmethod.
This method handles various numeric and string types cleanly. The fallback toRevision.VALID_REVISION_REGEX.test(value)is a good catch-all for strings.
60-69: Good handling ofUint8Arrayinputs.
ConvertingUint8Arrayto aTxtand then validating allows this class to remain flexible without duplicating logic.
72-79: Clear error handling and fallback.
Consistently throwingInvalidDataTypefor invalid inputs makes debugging simpler and fosters consistent error responses.
96-105: New static properties provide convenience.
ExposingNEXTandJUSTIFIEDas built-in revision instances helps unify usage and reduces manual string entry.packages/network/src/provider/utils/rpc-mapper/methods/eth_getCode/eth_getCode.ts (4)
7-7: Imports updated appropriately.
Switching fromBlockQuantityInputRPCtoDefaultBlockis consistent with the new design. IncludingDefaultBlockToRevisionfor block translation is logical.
9-9: Import ofAddressremains valuable.
No issues here; referencingAddressfrom@vechain/sdk-coreis necessary for the account bytecode retrieval.
42-42: Explicitly typed parameters.
Destructuring[address, block]as[string, DefaultBlock]clarifies usage for both maintainers and static analyzers.
48-48: Block translation logic is streamlined.
UsingDefaultBlockToRevision(block)aligns with the new block-handling approach. Be aware that your docstring mentions only 'latest' and 'finalized' are supported, yet the function now supports additional tags (e.g., 'pending', 'earliest'). Consider updating the docstring or restricting unsupported tags if necessary.Would you like a script to scan your documentation for references to supported block tags and highlight potential discrepancies?
packages/network/src/provider/utils/rpc-mapper/methods/eth_getBalance/eth_getBalance.ts (1)
7-7: LGTM! Clean implementation of standardized block parameter handling.The changes effectively implement the standardized block parameter handling using
DefaultBlocktype andDefaultBlockToRevisionfunction, while maintaining robust error handling and input validation.Also applies to: 42-42, 48-48
packages/network/src/provider/utils/rpc-mapper/methods/eth_getStorageAt/eth_getStorageAt.ts (1)
3-3: LGTM! Consistent implementation of block parameter handling.The changes align well with the standardized approach used in other RPC methods, maintaining robust error handling and input validation.
Also applies to: 47-47, 55-55
packages/network/src/provider/utils/rpc-mapper/methods/eth_call/eth_call.ts (1)
6-6: LGTM! Well-implemented block parameter handling with proper type conversion.The changes effectively implement the standardized block parameter handling, with proper conversion to string for the simulateTransaction API. The use of
satisfieskeyword enhances type safety.Also applies to: 43-43, 56-56
packages/network/src/provider/utils/rpc-mapper/methods/eth_getBlockByHash/eth_getBlockByHash.ts (1)
44-59: LGTM! Well-structured block retrieval implementation.The implementation effectively handles block retrieval by hash, with proper conditional logic for transaction details and chain ID retrieval. The error handling is robust and the code is well-documented.
packages/network/src/provider/utils/rpc-mapper/methods/eth_estimateGas/eth_estimateGas.ts (2)
11-11: LGTM! Improved type safety and standardization.The changes enhance parameter handling by:
- Using the new
DefaultBlocktype for better type safety- Standardizing block parameter handling with
DefaultBlockToRevisionAlso applies to: 44-48
60-60: Verify revision string conversion.The
revision.toString()call assumes the revision object is valid. WhileDefaultBlockToRevisionshould handle validation, it's good practice to add error handling.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check for error handling in DefaultBlockToRevision implementation ast-grep --pattern 'function DefaultBlockToRevision($_) { $$$ throw $$$ $$$ }'Length of output: 86
I'll run an additional search to inspect the implementation of
DefaultBlockToRevisionin more detail (showing some context) to see if it performs any input validation or error handling. This will help us verify whether the use ofrevision.toString()is safe.
🏁 Script executed:
#!/bin/bash rg -C 10 "function DefaultBlockToRevision" -nLength of output: 47
Revision conversion may fail without proper error handling
After searching for error handling code related to the
DefaultBlockToRevisionfunction, no evidence was found that it includes safeguards (such as checking the validity of the input or using a try-catch block). This means that the call torevision.toString()ineth_estimateGas.tsrelies entirely on the assumption that the revision object is always valid. In light of this, it's advisable to add error handling or a verification step to protect against potentially invalid revision values.
- No error handling (e.g., throw statements) was found in the implementation of
DefaultBlockToRevision.- The direct use of
revision.toString()may lead to runtime errors if the revision object is not valid.- Consider adding a try-catch block or validating the revision object before conversion.
packages/network/src/provider/utils/rpc-mapper/methods/eth_getBlockByNumber/eth_getBlockByNumber.ts (1)
45-46: LGTM! Consistent block parameter handling.The changes align with the standardized approach using
DefaultBlockandDefaultBlockToRevision.Also applies to: 56-57
packages/network/tests/provider/rpc-mapper/methods/eth_call/eth_call.mock.solo.test.ts (1)
58-86: Verify error type in test assertions.Both tests expect
JSONRPCInternalErrorbut the test descriptions mentionJSONRPCInvalidParams. This inconsistency should be resolved.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check the error type thrown by DefaultBlockToRevision for invalid inputs ast-grep --pattern 'function DefaultBlockToRevision($_) { $$$ throw new JSONRPCInvalidParams($$$) $$$ }'Length of output: 112
Action Required: Update Test Descriptions to Match the Thrown Error
Our initial search for any occurrence of a throw statement for JSONRPCInvalidParams in the implementation (via DefaultBlockToRevision) returned no results. This indicates that the implementation isn’t actually throwing a JSONRPCInvalidParams error—instead, it’s throwing JSONRPCInternalError (as the tests assert). In other words, the error type in the test description (JSONRPCInvalidParams) is inconsistent with the error type actually thrown by the code (JSONRPCInternalError).
To resolve this, please update the test descriptions so they correctly indicate that JSONRPCInternalError is expected for an invalid default block parameter.
- File: packages/network/tests/provider/rpc-mapper/methods/eth_call/eth_call.mock.solo.test.ts
- Location: Lines 58-86
packages/core/tests/vcdm/Revision.unit.test.ts (1)
34-34: LGTM! Improved test coverage and clarity.The changes enhance the test suite by:
- Clarifying the test description for invalid inputs
- Adding coverage for the new 'next' block tag
Also applies to: 57-59
packages/network/src/provider/utils/rpc-mapper/rpc-mapper.ts (1)
97-99: Great addition of the generic type definition!The introduction of
MethodHandlerTypewith generic parameters improves type safety and ensures consistency across RPC method implementations. The usage inRPCMethodsMap's return type further enhances type checking.Also applies to: 115-115
packages/rpc-proxy/README.md (1)
264-276: Excellent documentation addition!The new "RPC to VeChain Mappings" section provides clear and essential information about parameter translations. The table format makes it easy for developers to understand how RPC parameters map to VeChain parameters.
🧰 Tools
🪛 LanguageTool
[grammar] ~275-~275: A determiner may be missing.
Context: ... | best block | | earliest block | block n...(THE_SUPERLATIVE)
packages/network/src/provider/utils/rpc-mapper/methods/eth_getBlockByHash/eth_getBlockByHash.ts
Outdated
Show resolved
Hide resolved
packages/network/tests/provider/rpc-mapper/methods/eth_call/eth_call.mock.solo.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/network/src/provider/utils/rpc-mapper/methods/eth_getBlockByHash/eth_getBlockByHash.ts (1)
42-69: Consider these readability improvements.While the implementation is functionally sound, we could improve its readability:
- Move the chainId initialization closer to where it's used
- Simplify the nested ternary operations
Here's a suggested refactor:
try { const [blockHash, isTxDetail] = params as [string, boolean]; - - let chainId: string = '0x0'; - - // If the transaction detail flag is set, we need to get the chain id - if (isTxDetail) { - chainId = await ethChainId(thorClient); - } - const block = isTxDetail ? await thorClient.blocks.getBlockExpanded(blockHash) : await thorClient.blocks.getBlockCompressed(blockHash); - return block !== null - ? blocksFormatter.formatToRPCStandard(block, chainId) - : null; + if (block === null) { + return null; + } + + const chainId = isTxDetail ? await ethChainId(thorClient) : '0x0'; + return blocksFormatter.formatToRPCStandard(block, chainId); } catch (e) {packages/rpc-proxy/README.md (1)
264-276: Great addition of RPC to VeChain mappings!This section provides valuable documentation for developers working with the RPC proxy. The mappings clearly show how RPC block parameters are translated to their VeChain equivalents.
Consider enhancing this section with:
- A brief explanation of why these mappings are important for developers
- Additional context for non-obvious mappings (e.g., why 'pending' maps to 'best')
Here's a suggested enhancement:
## RPC to VeChain Mappings +The RPC proxy translates Ethereum-style RPC parameters to their VeChain equivalents to ensure compatibility. Understanding these mappings is crucial for developers working with both ecosystems. + The following mappings are performed by the RPC proxy | RPC Parameter | VeChain Parameter | |----------------------------------------|-----------------------| | block hash | block id | | latest block | best block | | safe block | justified block | | finalized block | finalized block | | pending block | best block | | earliest block | block number 0 | + +Note: 'pending' block maps to 'best' block as VeChain's consensus mechanism differs from Ethereum's, and the best block represents the most recent state.🧰 Tools
🪛 LanguageTool
[grammar] ~275-~275: A determiner may be missing.
Context: ... | best block | | earliest block | block n...(THE_SUPERLATIVE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/network/src/provider/utils/rpc-mapper/methods/eth_getBlockByHash/eth_getBlockByHash.ts(3 hunks)packages/rpc-proxy/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/rpc-proxy/README.md
[grammar] ~275-~275: A determiner may be missing.
Context: ... | best block | | earliest block | block n...
(THE_SUPERLATIVE)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: rpc-proxy / test / test
- GitHub Check: unit-integration-test / Build & Lint (latest)
- GitHub Check: unit-integration-test-browser / Build & Lint (latest)
- GitHub Check: unit-integration-test / Build & Lint (lts/*)
- GitHub Check: unit-integration-test-browser / Build & Lint (lts/*)
- GitHub Check: unit-integration-test / Build & Lint (18)
- GitHub Check: unit-integration-test-browser / Build & Lint (18)
- GitHub Check: test-apps / Install and test example apps
- GitHub Check: install-build / Build & Lint
- GitHub Check: Execute doc examples
🔇 Additional comments (2)
packages/network/src/provider/utils/rpc-mapper/methods/eth_getBlockByHash/eth_getBlockByHash.ts (2)
1-24: Well-documented implementation with clear imports!The documentation is thorough and the imports are clean. The added note about Ethereum block hash mapping to Thor block ID is particularly helpful for maintainers.
29-40: Robust input validation with descriptive error messages!The validation logic thoroughly checks parameter count, types, and block hash validity. The error messages are helpful and include documentation references.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rpc-proxy/README.md (1)
271-276: Consider adding article "the" for consistency.For better readability and grammatical consistency, consider adding the article "the" before block parameters in the VeChain Parameter column.
Apply this diff to improve consistency:
-| block hash | block id | -| latest block | best block | -| safe block | justified block | -| finalized block | finalized block | -| pending block | best block | -| earliest block | block number 0 | +| block hash | the block id | +| latest block | the best block | +| safe block | the justified block | +| finalized block | the finalized block | +| pending block | the best block | +| earliest block | the block number 0 |🧰 Tools
🪛 LanguageTool
[grammar] ~276-~276: A determiner may be missing.
Context: ... | best block | | earliest block | block n...(THE_SUPERLATIVE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rpc-proxy/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/rpc-proxy/README.md
[grammar] ~276-~276: A determiner may be missing.
Context: ... | best block | | earliest block | block n...
(THE_SUPERLATIVE)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: rpc-proxy / docker / docker
- GitHub Check: rpc-proxy / test / test
- GitHub Check: unit-integration-test-browser / Build & Lint (latest)
- GitHub Check: unit-integration-test-browser / Build & Lint (lts/*)
- GitHub Check: unit-integration-test / Build & Lint (latest)
- GitHub Check: unit-integration-test-browser / Build & Lint (18)
- GitHub Check: unit-integration-test / Build & Lint (lts/*)
- GitHub Check: unit-integration-test / Build & Lint (18)
- GitHub Check: test-apps / Install and test example apps
- GitHub Check: install-build / Build & Lint
- GitHub Check: Execute doc examples
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/rpc-proxy/README.md (1)
265-277: Great addition of RPC to VeChain parameter mappings!This documentation enhancement clearly maps RPC block parameters to their VeChain equivalents, which is essential for developers integrating with the VeChain blockchain. The mappings align well with the DefaultBlock type implementation and provide crucial clarity for cross-chain development.
🧰 Tools
🪛 LanguageTool
[grammar] ~276-~276: A determiner may be missing.
Context: ... | best block | | earliest block | block n...(THE_SUPERLATIVE)
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/network/tests/provider/rpc-mapper/methods/eth_call/fixture.ts (1)
105-133: 🛠️ Refactor suggestionRemove duplicate test case.
The test cases at lines 105-118 and 120-133 are identical. Consider removing one of them to maintain a clean and efficient test suite.
Apply this diff to remove the duplicate:
{ description: 'Send complex transaction object.', input: [ { from: '0x7487d912d03ab9de786278f679592b3730bdd540', to: '0xd46e8dd67c5d32be8058bb8eb970870f07244567', gas: 30432, gasPrice: '0x9184e72a000', value: '0x9184e72a', data: '0xd46e8dd67c5d32be8d46e8dd67c5d32be8058bb8eb970870f072445675058bb8eb970870f072445675' }, 'latest' ], expected: '0x' }, - { - description: 'Send complex transaction object.', - input: [ - { - from: '0x7487d912d03ab9de786278f679592b3730bdd540', - to: '0xd46e8dd67c5d32be8058bb8eb970870f07244567', - gas: 30432, - gasPrice: '0x9184e72a000', - value: '0x9184e72a', - data: '0xd46e8dd67c5d32be8d46e8dd67c5d32be8058bb8eb970870f072445675058bb8eb970870f072445675' - }, - 'latest' - ], - expected: '0x' - }
🧹 Nitpick comments (9)
packages/network/tests/provider/rpc-mapper/methods/eth_estimateGas/fixture.ts (1)
1-67: Consider adding more edge cases for block reference validation.While the current test coverage is good, consider adding more edge cases to thoroughly validate block reference handling:
- Invalid block number formats
- Non-existent block references
- Special block tags beyond 'latest'
Would you like me to help generate additional test cases for comprehensive block reference validation?
packages/network/src/provider/utils/rpc-mapper/methods/eth_getBlockTransactionCountByNumber/eth_getBlockTransactionCountByNumber.ts (2)
6-16: Update parameter description in JSDoc.The parameter description incorrectly mentions "block hash" instead of "block number". This should be corrected to maintain accurate documentation.
- * * params[0]: The block hash of block to get. + * * params[0]: The block number or tag (e.g., "latest", "earliest", "pending").
29-29: Consider performance optimization for transaction count.The second parameter
trueinethGetBlockByNumbermeans it will fetch full transaction objects instead of just transaction hashes. Since we only need the count, this is inefficient. Consider usingfalseto fetch only transaction hashes, which would reduce network payload and processing time.- const block = await ethGetBlockByNumber(thorClient, [params[0], true]); + const block = await ethGetBlockByNumber(thorClient, [params[0], false]);packages/network/tests/provider/rpc-mapper/methods/eth_getBlockTransactionCountByNumber/fixture.ts (1)
3-10: Consider adding more test cases for better coverage.The current test case is well-structured, but consider adding more cases to test different scenarios:
- Latest block ('latest')
- Earliest block ('earliest')
- Pending block ('pending')
- Block with multiple transactions
- Block with zero transactions
packages/network/tests/provider/rpc-mapper/methods/eth_getBlockTransactionCountByNumber/eth_getBlockTransactionCountByNumber.testnet.test.ts (1)
8-68: LGTM! Clean test structure with a minor suggestion.The test implementation is well-organized and correctly uses the updated fixture names. Consider enhancing error messages in negative test cases by including the actual error details:
await expect( async () => await RPCMethodsMap(thorClient)[ RPC_METHODS.eth_getBlockTransactionCountByNumber ](params) -).rejects.toThrowError(expectedError); +).rejects.toThrowError(expect.objectContaining({ + name: expectedError.name, + message: expect.any(String) +}));This would help in debugging by providing more context when tests fail.
packages/network/src/provider/utils/const/blocks/blocks.ts (2)
4-10: Consider using a more specific type for hex strings.The current type allows any string after '0x'. Consider using a more specific type to ensure valid hex values.
type DefaultBlock = - | `0x${string}` + | `0x${string & Record<number, string>}` | 'latest' | 'earliest' | 'pending' | 'safe' | 'finalized';
32-57: Consider using a map for block tag mappings.The current if-else chain could be replaced with a map for better maintainability and readability.
+const blockTagToRevision: Record<string, string | number> = { + earliest: 0, + safe: 'justified', + finalized: 'finalized', + latest: 'best', + pending: 'best' +}; const DefaultBlockToRevision = (defaultBlock: DefaultBlock): Revision => { if (HexUInt.isValid(defaultBlock)) { return Revision.of(HexUInt.of(defaultBlock).n.toString()); } if (!defaultBlockTags.includes(defaultBlock)) { const defaultBlockValue = defaultBlock.toString(); throw new JSONRPCInvalidDefaultBlock( 'DefaultBlockToRevision', `Invalid default block: ${defaultBlockValue}`, defaultBlockValue, null ); } - if (defaultBlock === 'earliest') { - return Revision.of(HexUInt.of(0)); - } else if (defaultBlock === 'safe') { - return Revision.of('justified'); - } else if (defaultBlock === 'finalized') { - return Revision.of('finalized'); - } else { - return Revision.of('best'); - } + const revision = blockTagToRevision[defaultBlock]; + return Revision.of(revision); };packages/errors/src/available-errors/provider/provider.ts (1)
121-127: Consider extending JSONRPCProviderError for consistency.The new error class could follow the pattern of other JSON-RPC errors by extending JSONRPCProviderError and including an error code.
-class JSONRPCInvalidDefaultBlock extends VechainSDKError<string> {} +class JSONRPCInvalidDefaultBlock extends JSONRPCProviderError { + constructor( + readonly methodName: string, + message: string, + data: string, + readonly innerError?: unknown + ) { + super(methodName, -32604, message, { value: data }, innerError); + } +}packages/rpc-proxy/README.md (1)
279-286: Fix grammatical and formatting issues in the Transaction Conversions section.There are several minor issues to address:
-## Transaction Coversions +## Transaction Conversions -The method `eth_sendTransaction` requires the input to be a VeChain transaction object, not a Ethereum transaction object -This method signs the transaction using the configured PK, before passing it on to VeChain Thor +The method `eth_sendTransaction` requires the input to be a VeChain transaction object, not an Ethereum transaction object. +This method signs the transaction using the configured PK before passing it on to VeChain Thor. -For method `eth_sendRawTransaction` the signed encoded raw transaction parameter must be a vechain transaction object -This method cannot convert a signed Ethereum transaction to a signed VeChain transaction +For method `eth_sendRawTransaction`, the signed encoded raw transaction parameter must be a VeChain transaction object. +This method cannot convert a signed Ethereum transaction to a signed VeChain transaction.🧰 Tools
🪛 LanguageTool
[misspelling] ~281-~281: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...to be a VeChain transaction object, not a Ethereum transaction object This meth...(EN_A_VS_AN)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/errors/src/available-errors/provider/provider.ts(2 hunks)packages/network/src/provider/utils/const/blocks/blocks.ts(1 hunks)packages/network/src/provider/utils/rpc-mapper/methods/eth_estimateGas/eth_estimateGas.ts(4 hunks)packages/network/src/provider/utils/rpc-mapper/methods/eth_getBlockTransactionCountByNumber/eth_getBlockTransactionCountByNumber.ts(1 hunks)packages/network/tests/provider/rpc-mapper/methods/eth_call/fixture.ts(1 hunks)packages/network/tests/provider/rpc-mapper/methods/eth_estimateGas/fixture.ts(1 hunks)packages/network/tests/provider/rpc-mapper/methods/eth_getBalance/fixture.ts(0 hunks)packages/network/tests/provider/rpc-mapper/methods/eth_getBlockByHash/fixture.ts(1 hunks)packages/network/tests/provider/rpc-mapper/methods/eth_getBlockTransactionCountByNumber/eth_getBlockTransactionCountByNumber.testnet.test.ts(3 hunks)packages/network/tests/provider/rpc-mapper/methods/eth_getBlockTransactionCountByNumber/fixture.ts(2 hunks)packages/rpc-proxy/README.md(1 hunks)
💤 Files with no reviewable changes (1)
- packages/network/tests/provider/rpc-mapper/methods/eth_getBalance/fixture.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/network/src/provider/utils/rpc-mapper/methods/eth_estimateGas/eth_estimateGas.ts
🧰 Additional context used
🪛 LanguageTool
packages/rpc-proxy/README.md
[grammar] ~276-~276: A determiner may be missing.
Context: ... | best block | | earliest block | block n...
(THE_SUPERLATIVE)
[misspelling] ~281-~281: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...to be a VeChain transaction object, not a Ethereum transaction object This meth...
(EN_A_VS_AN)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: rpc-proxy / test / test
- GitHub Check: unit-integration-test-browser / Build & Lint (latest)
- GitHub Check: unit-integration-test / Build & Lint (latest)
- GitHub Check: unit-integration-test-browser / Build & Lint (lts/*)
- GitHub Check: unit-integration-test / Build & Lint (lts/*)
- GitHub Check: unit-integration-test-browser / Build & Lint (18)
- GitHub Check: unit-integration-test / Build & Lint (18)
- GitHub Check: test-apps / Install and test example apps
- GitHub Check: install-build / Build & Lint
- GitHub Check: Execute doc examples
🔇 Additional comments (5)
packages/network/tests/provider/rpc-mapper/methods/eth_estimateGas/fixture.ts (1)
55-63: LGTM! Good addition of error handling test case.The new test case properly validates the error handling for missing block reference, which aligns well with the BLK-01 audit improvements. The expected
JSONRPCInvalidParamserror is appropriate for this scenario.packages/network/tests/provider/rpc-mapper/methods/eth_call/fixture.ts (1)
64-74: LGTM! Block parameter format change aligns with standardization efforts.The change from object-based block number to hex string format (
'0x0') aligns with the broader effort to standardize block parameter handling across RPC methods.packages/network/tests/provider/rpc-mapper/methods/eth_getBlockTransactionCountByNumber/fixture.ts (1)
12-33: LGTM! Clean and consistent naming.The updated comment accurately reflects the RPC method, and the variable renaming improves code clarity.
packages/network/tests/provider/rpc-mapper/methods/eth_getBlockByHash/fixture.ts (2)
1-76: Excellent test coverage and structure!The test suite demonstrates thorough coverage with well-organized test cases:
- Comprehensive valid scenarios including zero block and transaction details
- Robust invalid parameter handling
- Clear descriptions and expected results
- Good separation of valid and invalid test cases
38-45: LGTM! Clear test case for non-existent block.The test case is well-documented with a clear description and explicit comment indicating that this is an invalid block hash for testnet. The expected null result aligns with the standard Ethereum RPC behavior for non-existent blocks.
To ensure this block hash is consistently invalid across different test environments, let's verify it:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that the block hash doesn't exist in any test files or fixtures # except for negative test cases rg --type ts --type js -C 3 '0x00000000851caf3cfdb6e899cf5958bfb1ac3413d346d43539627e6be7ec1b4a' | grep -v 'Should get null if block does not exist'Length of output: 1350
Note on the Block Hash Usage
Our search shows that the block hash
0x00000000851caf3cfdb6e899cf5958bfb1ac3413d346d43539627e6be7ec1b4a
is used both as the “invalid block hash for testnet” in the test fixture and as theidfor the mainnet genesis block in the network constants. This is acceptable as long as environments are clearly separated—namely, the mainnet configuration (inpackages/core/src/utils/const/network.ts) isn’t used during testnet negative testing. The current test case’s intent is to ensure that querying testnet with a block hash (valid on mainnet) returnsnullfor non-existent blocks.Given that our search confirms this usage is isolated to the relevant negative test case and the mainnet configuration, the update remains appropriate provided that the test environments are correctly isolated.
LGTM!
No changes are required as both usages serve their intended contexts.
Test CoverageSummary
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docker/rpc-proxy/Dockerfile (3)
22-23: Relocate OpenSSL upgrade for better caching.The OpenSSL upgrade step should be moved before the dependency installation to leverage Docker's layer caching effectively. This ensures that package updates don't invalidate the cached
node_modules.# Set the working directory in the builder stage WORKDIR /app +# Upgrade openssl +RUN apk update && apk upgrade openssl + # Copy the dependencies file to the working directory COPY ./packages/rpc-proxy ./packages/rpc-proxy
2-2: Consider pinning Alpine version for better reproducibility.While Node.js version is pinned, the Alpine version could be more specific. Currently using
alpine3.20, but consider pinning to a specific patch version for better build reproducibility.-FROM node:20.17-alpine3.20 AS builder +FROM node:20.17-alpine3.20.0 AS builder -FROM node:20.17.0-alpine3.20 AS runner +FROM node:20.17.0-alpine3.20.0 AS runnerAlso applies to: 31-31
19-20: Consider using build arguments for better flexibility.The build step could benefit from build arguments to control Node.js environment variables like
NODE_ENVand enable build optimizations.+ARG NODE_ENV=production +ENV NODE_ENV=${NODE_ENV} # Install all the dependencies and build the app -RUN yarn install && yarn build +RUN yarn install --frozen-lockfile && yarn build
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docker/rpc-proxy/Dockerfile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: rpc-proxy / test / test
- GitHub Check: rpc-proxy / docker / docker
- GitHub Check: unit-integration-test-browser / Build & Lint (latest)
- GitHub Check: unit-integration-test / Build & Lint (latest)
- GitHub Check: unit-integration-test-browser / Build & Lint (lts/*)
- GitHub Check: unit-integration-test / Build & Lint (lts/*)
- GitHub Check: unit-integration-test-browser / Build & Lint (18)
- GitHub Check: unit-integration-test / Build & Lint (18)
- GitHub Check: test-apps / Install and test example apps
- GitHub Check: install-build / Build & Lint
- GitHub Check: Execute doc examples
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
docker/rpc-proxy/Dockerfile (1)
51-53: LGTM! Excellent security practices.Great job implementing security best practices:
- Using
--frozen-lockfileto ensure dependency consistency- Installing only production dependencies
- Creating a non-root user
- Cleaning yarn cache
Description
Update for audit review item BLK-01
Fixes #1666
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
eth_callmethod.eth_getBlockTransactionCountByNumbermethod.