1658 bnc - part 1 #1777
1658 bnc - part 1 #1777
Conversation
WalkthroughThis pull request introduces a comprehensive terminology update across the VeChain SDK documentation and codebase, replacing the term "delegator" with "gas-payer" to provide clearer and more precise language around transaction fee delegation. The changes span multiple files in documentation and implementation, focusing on renaming variables, updating comments, and ensuring consistent terminology that better reflects the role of the account responsible for paying transaction fees. Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (10)
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: 3
🧹 Nitpick comments (4)
docs/examples/transactions/fee-delegation.ts (1)
64-64: LGTM: Consistent terminology update in fee delegation exampleThe variable renames from delegator to gas-payer are consistent and maintain the example's functionality.
Consider adding a comment explaining the role of the gas payer in fee delegation for better documentation:
const nodeDelegate = HDKey.fromMnemonic(Mnemonic.of()); +// Generate the private key for the account that will pay for transaction fees const gasPayerPrivateKey = nodeDelegate.privateKey;Also applies to: 68-68, 74-74
packages/network/src/signer/signers/vechain-private-key-signer/vechain-private-key-signer.ts (1)
167-167: Update remaining parameter documentation.While the implementation has been updated, some JSDoc parameter descriptions still reference the old terminology (
delegatorUrl,delegatorPrivateKey).Consider updating the parameter documentation in the JSDoc comments to use the new terminology consistently.
Also applies to: 174-174, 182-187, 206-210, 217-222, 226-228
docs/contracts.md (1)
206-215: Enhance the deprecation notice for better clarityWhile the deprecation notice is good, it could be more informative for users.
Consider expanding the deprecation notice:
- // The term `delegator` will be deprecated soon and renamed `gasPayer`. + // DEPRECATED: The term `delegator` is being replaced with `gasPayer` to better reflect its role in fee payment. + // The `delegator` field will be removed in a future version. Please use `gasPayer` instead.docs/transactions.md (1)
Line range hint
467-596: Consider adding a terminology transition guideWhile the documentation updates are thorough, users might benefit from additional context about the terminology change.
Consider adding a note at the beginning of the fee delegation section:
> Note: Recent versions of the SDK use the term "gas-payer" instead of "delegator" to better reflect the role's responsibility in transaction fee payment. This change affects only the terminology; the underlying functionality remains the same.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
docs/contracts.md(2 hunks)docs/examples/contracts/contract-delegation-ERC20.ts(4 hunks)docs/examples/transactions/fee-delegation.ts(2 hunks)docs/examples/transactions/full-flow-delegator-private-key.ts(3 hunks)docs/examples/transactions/full-flow-delegator-url.ts(3 hunks)docs/templates/contracts.md(1 hunks)docs/transactions.md(5 hunks)packages/core/tests/transaction/Transaction.unit.test.ts(16 hunks)packages/errors/src/available-errors/transaction/transaction.ts(1 hunks)packages/errors/tests/available-errors/transaction.unit.test.ts(2 hunks)packages/network/src/signer/signers/vechain-abstract-signer/vechain-abstract-signer.ts(1 hunks)packages/network/src/signer/signers/vechain-private-key-signer/vechain-private-key-signer.ts(4 hunks)packages/network/src/thor-client/transactions/helpers/delegation-handler.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/templates/contracts.md
- packages/network/src/signer/signers/vechain-abstract-signer/vechain-abstract-signer.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Analyze (javascript-typescript)
- 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-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 (13)
docs/examples/transactions/full-flow-delegator-url.ts (1)
38-39: LGTM: Clear variable renamingThe account variable rename is clear and consistent with the new terminology.
docs/examples/transactions/full-flow-delegator-private-key.ts (2)
32-33: LGTM: Clear account type definition and renamingThe account type definition and renaming are clear and consistent with the new terminology.
123-123: LGTM: Updated assertion using new terminologyThe test assertion correctly uses the new
gasPayerproperty.packages/errors/tests/available-errors/transaction.unit.test.ts (1)
69-69: LGTM! Consistent terminology update in test data.The renaming from
delegatorUrltogasPayerUrlin the test parameters aligns with the broader terminology update across the SDK.Also applies to: 87-88
packages/network/src/thor-client/transactions/helpers/delegation-handler.ts (1)
51-51: LGTM! Error property renamed for consistency.The error object property has been updated from
delegatorUrltogasPayerUrlto maintain consistency with the new terminology.Consider tracking remaining "delegation" terms for future updates:
✅ Verification successful
Terminology update scope extends beyond this PR
The current property rename from
delegatorUrltogasPayerUrlis correct and consistent with the immediate context. The term "delegation" is used extensively across multiple packages, including core functionality, tests, and documentation. This suggests that a complete terminology update should be handled as a separate, coordinated effort.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find remaining instances of "delegation" in file/class names rg -l "delegation" --type tsLength of output: 2408
packages/network/src/signer/signers/vechain-private-key-signer/vechain-private-key-signer.ts (1)
97-103: LGTM! Variable renaming is consistent.The variable renaming from
delegatortogasPayermaintains consistency with the new terminology.Also applies to: 111-111
docs/examples/contracts/contract-delegation-ERC20.ts (2)
26-27: LGTM! Clear variable renaming with deprecation notice.The changes are well-documented:
- Variable renamed from
delegatorAccounttogasPayerAccount- Helpful deprecation notice added to inform users about the upcoming terminology change
Also applies to: 39-42
73-73: LGTM! Consistent variable naming in test assertions.The test assertions maintain consistency with the new terminology.
packages/core/tests/transaction/Transaction.unit.test.ts (3)
20-26: LGTM! Consistent renaming of delegator-related constantsThe renaming from
DelegatorPrivateKeyFixtoGasPayerPrivateKeyFixandDelegatorFixtoGasPayerFixaligns with the PR's objective to update terminology.
236-236: LGTM! Consistent usage of new gas-payer terminology in test casesThe test cases have been updated to use the new
GasPayerFixreferences while maintaining the same test logic and coverage.Also applies to: 279-279, 307-307, 424-424, 474-474
Line range hint
674-715: LGTM! Correct preservation of "delegated" transaction terminologyThe test method names correctly maintain the use of "delegated" when referring to transaction types while updating role-specific terms to "gas-payer".
docs/transactions.md (2)
467-468: LGTM! Clear and consistent terminology updates in documentationThe documentation has been updated to use the new gas-payer terminology consistently, with clear variable names and comments.
Also applies to: 488-490
575-576: LGTM! Consistent terminology in URL-based delegation exampleThe URL-based delegation example maintains consistency with the new gas-payer terminology.
Also applies to: 594-596
Test CoverageSummary
|
* fix: fix BNC... * fix: fix BNC... * fix: fix BNC... * fix: fix BNC... * fix: fix BNC... * fix: fix BNC... (cherry picked from commit 3eecea9)
Description
This PR removes the expression
delegatorto mean the gas-payer of delegated transactions inpackages/core/src/transactionpackages/network/src/signer/signersThe misleading
delegatorexpression is still present in other places of the the SDK, the refactoring to correct terminology is the goal of following PRs.Fixes # BNC
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:soloyarn test:unitTest Configuration:
Summary by CodeRabbit
Documentation
Refactor
Tests