chore: src-01 and sro-01 #1745
chore: src-01 and sro-01 #1745
Conversation
WalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Clause
participant FungibleToken
participant Transaction
User->>Clause: transferToken(tokenAddress, recipientAddress, amount)
Clause->>FungibleToken: Validate token amount
FungibleToken-->>Clause: Validated token
Clause->>Transaction: Create transaction with token transfer
Transaction-->>User: Transaction confirmation
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/core/src/vcdm/currency/FungibleToken.ts (1)
1-39: Consider adding input validation for the decimal parameter.
Currently, there's no explicit check to ensuredecimalsis non-negative (or within a sensible range). Passing an unexpected or negative value might cause unintended behavior.protected constructor( symbol: Txt, value: FixedPointNumber, - decimals: bigint = FungibleToken.WEI_DEFAULT + decimals: bigint = FungibleToken.WEI_DEFAULT, ) { + if (decimals < 0n) { + throw new Error('Decimal places cannot be negative'); + } super(symbol, value); this._decimals = decimals; this.wei = value.dp(this._decimals).scaledValue; }packages/core/tests/vcdm/currency/FungibleToken.unit.test.ts (1)
1-47: Test boundary conditions for decimals.
These tests validate typical scenarios (custom and default decimals). Consider adding boundary or extreme test cases (e.g., zero decimals, very large decimals, negative attempts, etc.) to further strengthen the reliability of this class.packages/core/src/transaction/Clause.ts (2)
225-225: Update JSDoc reference.
The current documentation references "VTHO" via@see VTHO.transferTokenTo, but the parameter now accepts a generalFungibleToken. Consider updating the reference to avoid confusion if you intend for any VIP-180 token to be transferred.- * @see VTHO.transferTokenTo + * @see FungibleToken.transferTo
225-225: Clarify integer vs. decimal amounts in JSDoc.
The JSDoc states "Throws an error if the amount is not a positive integer," but the check isisFinite()andisPositive(). If decimals are allowed, update the documentation for consistency.- * @throws {InvalidDataType} Throws an error if the amount is not a positive integer. + * @throws {InvalidDataType} Throws an error if the amount is not a finite positive number.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/core/src/transaction/Clause.ts(3 hunks)packages/core/src/vcdm/currency/FungibleToken.ts(1 hunks)packages/core/src/vcdm/currency/VTHO.ts(3 hunks)packages/core/tests/vcdm/currency/FungibleToken.unit.test.ts(1 hunks)packages/core/tests/vcdm/currency/VTHO.unit.test.ts(1 hunks)packages/network/src/provider/providers/hardhat-provider/hardhat-provider.ts(1 hunks)packages/network/src/signer/signers/vechain-abstract-signer/vechain-abstract-signer.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/network/src/signer/signers/vechain-abstract-signer/vechain-abstract-signer.ts
- packages/network/src/provider/providers/hardhat-provider/hardhat-provider.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- 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: Execute doc examples
🔇 Additional comments (3)
packages/core/tests/vcdm/currency/VTHO.unit.test.ts (1)
28-34: Test coverage looks solid!
This test correctly verifies theweivalue using the default of 18 decimals for VTHO. No issues found here.packages/core/src/transaction/Clause.ts (2)
235-235: Good extension to a broader token type.
Switching fromVTHOtoFungibleTokennicely broadens this method’s applicability to different VIP-180 tokens. This helps reduce code duplication and centralizes token handling.
18-18: Ensure the import path is valid.
This newly introduced import references a path that might not exist or might have been moved.Below is a script to search for the
FungibleTokendefinition and confirm that it’s located at../vcdm/currency/FungibleToken:✅ Verification successful
Import path is valid and correct
The file exists atpackages/core/src/vcdm/currency/FungibleToken.tsand exports theFungibleTokensymbol, making the import path../vcdm/currency/FungibleTokenvalid frompackages/core/src/transaction/Clause.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the file FungibleToken.ts exists in the specified path and includes the export for FungibleToken. fd FungibleToken.ts --exec cat {} | rg -A 10 -B 10 "export.*FungibleToken"Length of output: 350
Script:
#!/bin/bash # Show the full path of FungibleToken.ts fd --full-path FungibleToken.tsLength of output: 82
| import { FixedPointNumber } from '../FixedPointNumber'; | ||
| import { Txt } from '../Txt'; | ||
| import { Units } from './Units'; | ||
| import { FungibleToken } from './FungibleToken'; |
There was a problem hiding this comment.
💡 Codebase verification
The review comment is valid and the suggested fix is necessary
The Units enum defines valid values from 0 to 18, but there's no runtime validation to prevent invalid values that could cause the exponent calculation to fail. The suggested range check aligns with the codebase's unit system and prevents potential runtime errors.
- The fix should be applied to the
ofmethod inpackages/core/src/vcdm/currency/VTHO.ts - Additional test cases should be added to
packages/core/tests/vcdm/currency/VTHO.unit.test.tsto verify the validation
🔗 Analysis chain
Validate unit range for the of method.
When choosing a Units value that is significantly larger than VTHO_DECIMALS, (VTHO.VTHO_DECIMALS - BigInt(unit)) could become negative, potentially risking incorrect or unexpected scaling. Add a check to ensure unit remains in a valid range.
public static of(
value: bigint | number | string | FixedPointNumber,
unit: Units = Units.ether
): VTHO {
const fpn =
value instanceof FixedPointNumber
? value
: FixedPointNumber.of(value);
+ // Simple range check if unit is larger than 18 or negative.
+ if (BigInt(unit) > VTHO.VTHO_DECIMALS || BigInt(unit) < 0n) {
+ throw new Error('Invalid unit range for VTHO calculation');
+ }
return new VTHO(
fpn.div(
FixedPointNumber.of(10n ** (VTHO.VTHO_DECIMALS - BigInt(unit)))
)
);
}Also applies to: 11-11, 13-13, 26-26, 34-34, 56-58
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Get the complete VTHO implementation and related token files
echo "=== VTHO.ts ==="
cat packages/core/src/vcdm/currency/VTHO.ts
echo -e "\n=== Looking for similar token implementations ==="
fd ".*Token.*\.ts$" packages/core/src/vcdm/currency
echo -e "\n=== Searching for VTHO unit tests ==="
fd ".*VTHO.*test.*" packages/core
Length of output: 2516
Script:
#!/bin/bash
# Check the base class, Units enum, and tests
echo "=== FungibleToken.ts ==="
cat packages/core/src/vcdm/currency/FungibleToken.ts
echo -e "\n=== Units.ts ==="
cat packages/core/src/vcdm/currency/Units.ts
echo -e "\n=== VTHO.unit.test.ts ==="
cat packages/core/tests/vcdm/currency/VTHO.unit.test.ts
Length of output: 7040
Test CoverageSummary
|
|
closed as will redo in new PR |
Description
Audit findings SRC-01 and SRO-01
Fixes # (issue)
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
FungibleTokenclass to support flexible token handlingVTHOtoken implementation with improved decimal managementRefactor
Clausetransaction method to support generic fungible tokensVTHOclass to extendFungibleTokenTests
FungibleTokenandVTHOclassesDocumentation