1658 bnc part 2 #1788
1658 bnc part 2 #1788
Conversation
* fix: fr9 recommendations * fix: en_1 recommendations * fix: en_1 recommendations * fix: en_2 recommendations... * fix: en_2 recommendations... * fix: en_7 recommendations... * fix: en_7 recommendations... * fix: n_7 recommandations * fix: n_7 recommandations * Merge branch 'main' into 1656-fr9 * Merge branch '1661-en_2' into 1662-en_7 * fix: EN_7 * fix: EN_7 * fix: EN_7 --------- Co-authored-by: Fabio Rigamonti <73019897+fabiorigam@users.noreply.github.com> (cherry picked from commit f52a556)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Luca Nicola Debiasi <63785793+lucanicoladebiasi@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Luca Nicola Debiasi <63785793+lucanicoladebiasi@users.noreply.github.com>
Test CoverageSummary
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/rpc-proxy/src/utils/config-validator/config-validator.ts (1)
11-12: Update import statements to usegasPayernaming.The imported validation functions still use the old
delegatornaming convention.- isValidDelegatorPrivateKey, - isValidDelegatorUrl, + isValidGasPayerPrivateKey, + isValidGasPayerUrl,
♻️ Duplicate comments (3)
packages/rpc-proxy/src/utils/config-validator/config-validator.ts (3)
140-141: 🛠️ Refactor suggestionUpdate validation function call to use
gasPayernaming.The validation function call still uses the old
delegatornaming convention.- !isValidDelegatorPrivateKey(configFile.gasPayer.gasPayerPrivateKey) + !isValidGasPayerPrivateKey(configFile.gasPayer.gasPayerPrivateKey)
154-155: 🛠️ Refactor suggestionUpdate validation function call to use
gasPayernaming.The validation function call still uses the old
delegatornaming convention.- !isValidDelegatorUrl(configFile.gasPayer.gasPayerServiceUrl) + !isValidGasPayerUrl(configFile.gasPayer.gasPayerServiceUrl)
204-204:⚠️ Potential issueFix incorrect error message logic.
The error message states that gasPayer configuration must be removed when enableDelegation is false, but the actual check is for when enableDelegation is true and gasPayer is undefined.
- `Invalid configuration file: ${absolutePath}. The gasPayer configuration must be removed when enableDelegation is false`, + `Invalid configuration file: ${absolutePath}. The gasPayer configuration must be provided when enableDelegation is true`,
🧹 Nitpick comments (2)
docs/transactions.md (2)
371-373: Notice Missing Content in Complete Example "full-flow-no-gas-payer".
This example block is currently empty ("Content not found between specified comments."). If it is intended as a placeholder, please consider adding a short note or linking to the actual implementation so that developers know what to expect when they see "no gas payer" transactions.
369-370: Overall Documentation Consistency Check.
The updated complete examples now reference "gasPayer" rather than "delegator," which is in line with the PR objectives. Ensure that any remaining mentions of the old terminology throughout the document (and linked documentation) are updated accordingly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docker-compose.rpc-proxy.yml(7 hunks)docs/contracts.md(1 hunks)docs/examples/contracts/contract-delegation-ERC20.ts(2 hunks)docs/templates/transactions.md(2 hunks)docs/transactions.md(2 hunks)packages/hardhat-plugin/tests/hardhat-mock-projects/eth_getTransactionCount-default-value-project/hardhat.config.ts(1 hunks)packages/hardhat-plugin/tests/hardhat-mock-projects/eth_getTransactionCount-random-value-project/hardhat.config.ts(1 hunks)packages/hardhat-plugin/tests/hardhat-mock-projects/simple-vechain-hardhat-project/hardhat.config.ts(1 hunks)packages/network/src/thor-client/blocks/types.d.ts(1 hunks)packages/rpc-proxy/README.md(3 hunks)packages/rpc-proxy/src/utils/config-validator/config-validator.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/hardhat-plugin/tests/hardhat-mock-projects/eth_getTransactionCount-random-value-project/hardhat.config.ts
- packages/hardhat-plugin/tests/hardhat-mock-projects/eth_getTransactionCount-default-value-project/hardhat.config.ts
- packages/network/src/thor-client/blocks/types.d.ts
- docs/contracts.md
- docker-compose.rpc-proxy.yml
- docs/examples/contracts/contract-delegation-ERC20.ts
- packages/hardhat-plugin/tests/hardhat-mock-projects/simple-vechain-hardhat-project/hardhat.config.ts
- docs/templates/transactions.md
🧰 Additional context used
📓 Learnings (3)
📓 Common learnings
Learnt from: lucanicoladebiasi
PR: vechain/vechain-sdk-js#1777
File: packages/errors/src/available-errors/transaction/transaction.ts:33-33
Timestamp: 2025-01-30T17:15:49.377Z
Learning: PR #1777 is part of a phased approach to rename "delegator" terminology to "gas-payer" across the VeChain SDK. The changes are intentionally partial, with remaining occurrences to be addressed in subsequent PRs.
docs/transactions.md (1)
Learnt from: lucanicoladebiasi
PR: vechain/vechain-sdk-js#1777
File: packages/errors/src/available-errors/transaction/transaction.ts:33-33
Timestamp: 2025-01-30T17:15:49.377Z
Learning: PR #1777 is part of a phased approach to rename "delegator" terminology to "gas-payer" across the VeChain SDK. The changes are intentionally partial, with remaining occurrences to be addressed in subsequent PRs.
packages/rpc-proxy/README.md (1)
Learnt from: lucanicoladebiasi
PR: vechain/vechain-sdk-js#1788
File: packages/rpc-proxy/README.md:0-0
Timestamp: 2025-02-07T15:31:37.780Z
Learning: The CLI options in the RPC proxy continue to use "delegator" terminology (--delegatorPrivateKey, --delegatorUrl) temporarily, despite descriptions referring to "gasPayer". This will be updated in a future refactoring stage.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unit-integration-test-browser / Build & Lint (latest)
- GitHub Check: unit-integration-test-browser / Build & Lint (lts/*)
- GitHub Check: unit-integration-test-browser / Build & Lint (18)
- GitHub Check: unit-integration-test / Build & Lint (latest)
- GitHub Check: unit-integration-test / Build & Lint (lts/*)
- GitHub Check: unit-integration-test / Build & Lint (18)
🔇 Additional comments (5)
packages/rpc-proxy/src/utils/config-validator/config-validator.ts (2)
145-145: Update error message to usegasPayerterminology.The error message still references "delegator" instead of "gasPayer".
- `Invalid delegator private key in configuration file: ${absolutePath}. Delegator private key must be a valid private key`, + `Invalid gasPayer private key in configuration file: ${absolutePath}. GasPayer private key must be a valid private key`,
159-159: Update error message to usegasPayerterminology.The error message still references "delegator" instead of "gasPayer".
- `Invalid delegator url in configuration file: ${absolutePath}. Delegator url must be a valid URL`, + `Invalid gasPayer url in configuration file: ${absolutePath}. GasPayer url must be a valid URL`,docs/transactions.md (1)
116-124: Consistent Use of Updated Terminology in Fee Delegation Example.
In the Fee Delegation section, the code now correctly uses "gasPayerPrivateKey" and "gasPayerAddress" and the signing method “signAsSenderAndGasPayer.” The renaming aligns with the new naming conventions and improves clarity in the transaction process.🧰 Tools
🪛 LanguageTool
[grammar] ~116-~116: This phrase is duplicated. You should probably use “Fee Delegation” only once.
Context: ...ode(encodedRaw, true); ``` ## Example: Fee Delegation Fee delegation is a feature on the VeChainThor blockch...(PHRASE_REPETITION)
packages/rpc-proxy/README.md (2)
162-177: Update Configuration Example Key for GasPayer URLSimilarly, in this JSON example the "gasPayer" object still contains the key "delegatorUrl" rather than "gasPayerUrl". For consistency with the new naming convention, please update this inner property accordingly. Ensuring consistent naming in documentation is critical to prevent confusion during integration.
- "delegatorUrl": "https://sponsor-testnet.vechain.energy/by/..." + "gasPayerUrl": "https://sponsor-testnet.vechain.energy/by/..."
144-159: Update Configuration Example Key for GasPayer Private KeyIn this configuration example, the outer object has been renamed to "gasPayer", which is great. However, the inner property still uses the legacy name "delegatorPrivateKey". To fully align with the renaming objectives, please update it to "gasPayerPrivateKey". If this deliberate retention is meant for temporary backward compatibility, consider adding an inline note for clarity.
- "delegatorPrivateKey": "8f9290cc44c5fd2b95fe21d6ad6fe5fa9c177e1cd6f3b4c96a97b13e09eaa158" + "gasPayerPrivateKey": "8f9290cc44c5fd2b95fe21d6ad6fe5fa9c177e1cd6f3b4c96a97b13e09eaa158"
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/examples/transactions/full-flow-gas-payer-service-url.ts (1)
126-126: Consider removing the commented-out expectation.The commented-out expectation appears to be obsolete or unnecessary. If this test is no longer needed, it's better to remove it entirely rather than leaving it commented out.
-// expect(signedTx.gasPayer).toEqual(gasPayerAccount.address); ---
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/examples/transactions/full-flow-gas-payer-private-key.ts(3 hunks)docs/examples/transactions/full-flow-gas-payer-service-url.ts(3 hunks)docs/examples/transactions/full-flow-no-gas-payer.ts(2 hunks)docs/templates/transactions.md(2 hunks)docs/transactions.md(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/examples/transactions/full-flow-no-gas-payer.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/templates/transactions.md
- docs/transactions.md
⏰ 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-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
🔇 Additional comments (5)
docs/examples/transactions/full-flow-gas-payer-private-key.ts (3)
18-18: LGTM! Snippet identifier updated correctly.The snippet identifier has been updated to reflect the new terminology, maintaining consistency with the PR's objective.
53-55: LGTM! Configuration structure updated appropriately.The configuration object has been properly restructured to use the new
gasPayerterminology, maintaining the same functionality while improving clarity.
1-128: Excellent implementation with comprehensive test coverage.The example provides a well-structured, complete flow demonstrating gas payment functionality:
- Clear setup of accounts and provider
- Proper transaction building and signing
- Comprehensive verification steps
- Good error handling practices
The implementation serves as an excellent reference for developers.
docs/examples/transactions/full-flow-gas-payer-service-url.ts (2)
18-18: LGTM! Snippet identifier updated correctly.The snippet identifier has been updated from
FullFlowDelegatorUrlSnippettoFullFlowGasPayerServiceUrlSnippet, aligning with the PR's objective to rename delegator references.
57-59: LGTM! Configuration properties renamed correctly.The configuration properties have been updated to use the new terminology:
delegator→gasPayerdelegatorUrl→gasPayerServiceUrlThis change maintains consistency with the broader codebase updates.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
.github/workflows/rpc-proxy-docker.yml (5)
22-22: Nitpick: Unintended or Extra Whitespace?The tilde marker on line 22 appears to add an extra newline following the "Set up QEMU" step. Please confirm if this blank line is intentional—for readability or formatting—or if it was added inadvertently.
26-26: Nitpick: Verify Formatting ConsistencySimilar to line 22, the tilde marker on line 26 introduces an additional newline after the "Set up Docker Buildx" step. Verify that this extra whitespace is intentional and does not affect the YAML parsing or workflow execution.
39-39: Nitpick: Accidental Newline?Line 39 contains a tilde indicating a change after setting the "images" property. If this newline was added as a formatting choice, please ensure it aligns with the overall style guidelines for this workflow file.
51-54: Enhancement Suggestion: Expanded .trivyignore SetupThe updated "Create .trivyignore file" step now includes an additional CVE ("CVE-2024-13176") alongside the existing "CVE-2024-21538". This enhancement helps broaden the exclusions for the Trivy scan.
- Please confirm that ignoring these CVEs is acceptable given your security posture, and consider adding an inline comment explaining the rationale for ignoring these specific vulnerabilities.
Overall, this change is well implemented.
69-69: Nitpick: Trailing Newline in Environment BlockThe tilde marker on line 69 introduces an extra newline in the "Run Trivy Scan" step’s environment block. Please verify whether this additional whitespace is intended for clarity or might be removed to maintain consistency.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/rpc-proxy-docker.yml(4 hunks)docker/rpc-proxy/Dockerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docker/rpc-proxy/Dockerfile
⏰ 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)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rpc-proxy/README.md(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/rpc-proxy/README.md (1)
Learnt from: lucanicoladebiasi
PR: vechain/vechain-sdk-js#1788
File: packages/rpc-proxy/README.md:0-0
Timestamp: 2025-02-07T15:31:37.780Z
Learning: The CLI options in the RPC proxy continue to use "delegator" terminology (--delegatorPrivateKey, --delegatorUrl) temporarily, despite descriptions referring to "gasPayer". This will be updated in a future refactoring stage.
🪛 Gitleaks (8.21.2)
packages/rpc-proxy/README.md
156-156: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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: 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 (4)
packages/rpc-proxy/README.md (4)
155-157: Inconsistent terminology in configuration example.The configuration uses
gasPayeras the parent key but still usesdelegatorPrivateKeyas the child key. This mixed terminology could be confusing for users.Would you like me to generate a more consistent configuration example that aligns with the new terminology?
🧰 Tools
🪛 Gitleaks (8.21.2)
156-156: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
173-175: Inconsistent terminology in configuration example.Similar to the previous example, the configuration uses
gasPayeras the parent key butdelegatorUrlas the child key.Would you like me to generate a more consistent configuration example that aligns with the new terminology?
186-189: Enhance Docker build instructions with security disclaimer.The Docker build instructions have been improved with a clear disclaimer about replacing the default configuration. This is a good practice for preventing accidental use of known mnemonics in production.
90-98: CLI options terminology maintained as planned.The CLI options continue to use "delegator" terminology as intended, with plans to update in a future refactoring stage. This is consistent with the retrieved learnings.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~90-~90: Loose punctuation mark.
Context: ...e delegation --e, --enableDelegation: Whether to enable delegation. - `--dele...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~91-~91: Loose punctuation mark.
Context: ...legatorPrivateKey: The private key of the delegator. --d...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~92-~92: Loose punctuation mark.
Context: ...r. --d, --delegatorUrl <delegatorUrl>: The URL of the delegator. - -e.g.- ...(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
93-93: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
95-95: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
97-97: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
98-98: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
Description
Any reference to
delegatorin thenetworkpackage is renamed asgasPayer, included derived names for classes, properties and methods.Few
delegatorproperties are in test fixture until the remaining packages are sanitized as well.Fixes # BNC part 2 (network)
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
yarn test:examplesyarn test:soloTest Configuration:
Summary by CodeRabbit
Refactor
Documentation