Release 0.4.0: 6-Decimal Token Support and Dynamic Decimal Handling#5
Release 0.4.0: 6-Decimal Token Support and Dynamic Decimal Handling#5
Conversation
…ts. This includes the deletion of data model, implementation plan, quickstart guide, research notes, and other related files, streamlining the project structure as part of the deprecation process.
… Updated chainId validation, account generation, and comptroller deployment to utilize Unitroller. Documented test suite baseline and root cause investigation for failing tests.
…ormance and error handling. Update tests to utilize Rootstock Mainnet addresses and ensure compatibility with fresh unitroller deployments. Fix failing tests related to market listing and entry, enhancing overall test reliability.
…ts to remove skipped status. Adjusted test cases to ensure proper execution and accuracy, including changes to expected values and added precision in assertions. Updated borrowing logic to reflect correct amounts in tests.
…ability on forked networks. Updated test cases to correctly set pending implementations and verify comptroller addresses, ensuring compatibility with Rootstock Mainnet addresses.
…rket statuses. Corrected deprecation reasons for kRDOC/cRDOC and updated test addresses in deprecation.spec.js to ensure consistency and pass all tests successfully.
…ckstart.md file exists and is readable. Introduced explicit test to verify file content, resulting in all quickstart validation tests passing.
…restart requirement to prevent state issues. Update Unitroller tests to ensure transaction mining before verification, resulting in all tests passing. Documented final test suite baseline and verified test infrastructure configuration.
…cle integration. Enhance decimal detection and conversion utilities, ensuring backward compatibility with existing 18-decimal tokens. Update relevant Market and PriceOracle methods to handle mixed decimal calculations accurately. Add comprehensive integration tests for deposit, borrow, and USD value calculations, verifying correct behavior across scenarios.
…plementing decimal utility functions. Mark tasks T001 to T008 as completed, ensuring readiness for subsequent development phases.
…detectOracleDecimals` method to identify decimals for USDT and Moc adapters, and initialize `adapterDecimalsMap` for caching results. Update tasks in specs to reflect completion of these features.
… 18-decimal (MoC) and 30-decimal (USDT) price oracles. Update `detectOracleDecimals` method for accurate decimal detection and caching. Revise USD value calculations to handle mixed decimal scenarios correctly, ensuring backward compatibility with existing 18-decimal tokens. Add comprehensive integration tests for various operations, including deposit, borrow, and liquidity calculations, verifying correct behavior across all scenarios.
…ndex.js`. Update tasks in the specs to reflect the completion of decimal detection and adapter initialization for USDT and MoC, ensuring accurate handling of mixed decimal scenarios.
…zing the use of USDT0 (6-decimal) instead of the deprecated kUSDT (18-decimal). Revise README, specs, and quickstart files to clarify the handling of 6-decimal tokens with price oracles, ensuring accurate representation and backward compatibility. Enhance clarity on deprecated markets and their implications for new integrations.
…kets.spec.js`, including decimal detection, deposit, balance query, borrow, and repay functionalities. Mark the 'Markets operations' suite as skipped to focus on new tests for USDT0 token handling.
…asses to support dynamic decimal detection for ERC20 tokens. Implement async decimal initialization and parsing for accurate token amount handling. Update tests to verify correct behavior with 6-decimal tokens, ensuring backward compatibility and accurate USD value calculations.
…ait for improved readability and performance. Implement dynamic decimal detection for token and oracle values, ensuring accurate USD calculations. Update related tests to reflect changes in balance querying and precision handling for 6-decimal tokens.
…it for improved readability and dynamic decimal detection. Implement new borrow method in CErc20 class for borrowing amounts with proper token parsing. Update tests to verify correct balance calculations after borrowing, ensuring accurate handling of 6-decimal tokens.
…amic token factor calculation based on token decimals. Update repayBorrow method to use parsed token amounts instead of hardcoded values, ensuring accurate handling of token operations. Enhance tests to verify correct balance calculations after repayments with 6-decimal tokens.
…ecimal handling. Enhance tests to verify correct 6-decimal parsing during redemption, including partial and full redemptions, ensuring accurate balance calculations for users.
…ing for token operations. Update methods to replace hardcoded factors with calculations based on token decimals, ensuring accurate parsing and formatting for various token types. Enhance JSDoc comments for clarity on method functionalities and decimal handling. Update tests to verify correct behavior with 6-decimal tokens across all operations.
…ns with 6-decimal and 18-decimal tokens. Skip the previous 6-decimal token detection tests and add new tests for account liquidity, borrowing, and supply calculations across different decimal precisions, ensuring accurate handling of token operations.
…dling for token operations. Update getAccountLiquidity method to calculate market-specific liquidity with accurate token and oracle decimal detection. Refactor related methods for improved readability and precision. Update tests to verify correct behavior with 6-decimal and 18-decimal tokens across various operations.
…ne total supply calculations across markets. Replace redundant variables with a single priceUnderlying variable for improved clarity. Update tests to reflect changes in expected total supply values for accurate decimal handling.
… clarity and accuracy in borrow calculations. Replace underlyingBorrowAmount with priceUnderlying for better handling of target market pricing. Update tests to include new CRBTC market and ensure correct total borrows and supplies across multiple markets with accurate decimal handling.
…namic decimal handling for underlying amounts. Update liquidity tests to include new CRBTC market, ensuring accurate liquidity calculations across multiple markets. Adjust maxAllowedToWithdraw test for correct collateral factor application and liquidity assessment.
…ts for market operations and decimal detection. Update assertions to use closeTo for better precision in liquidity and borrowing calculations. Ensure accurate handling of token operations across different decimal precisions.
…0 tokens and dynamic decimal detection. Refactor relevant classes and methods for improved decimal handling, ensuring backward compatibility. Enhance documentation and test infrastructure, including comprehensive integration tests for new token operations.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughRelease version 0.4.0 introduces dynamic token decimal detection and handling across the protocol, particularly supporting 6-decimal ERC20 tokens like USDT. Core methods convert to async, price oracles normalize decimals, and tests migrate to Anvil for RSK Mainnet forking with comprehensive multi-market decimal precision coverage. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Oracle as PriceOracle
participant Adapter as OracleAdapter<br/>(USDT/MoC)
participant Market as Market
participant Token as ERC20 Token
participant Comptroller as Comptroller
App->>Market: Get balance (async)
Market->>Token: Call decimals()
Token-->>Market: tokenDecimals (6 or 18)
Market->>Adapter: detectOracleDecimals()
Adapter->>Adapter: Check DECIMAL_MULTIPLIER
Adapter-->>Market: oracleDecimals (18 or 30)
Note over Market: Cache decimals
Market->>Market: Fetch raw balance
Market->>Oracle: Get underlying price
Oracle->>Adapter: Fetch price (raw)
Adapter-->>Oracle: rawPrice
Oracle->>Oracle: Normalize by oracleDecimals
Oracle-->>Market: humanReadablePrice
Market->>Market: Calculate balance in USD<br/>(balance / 10^tokenDecimals) * price
Market-->>App: { underlying, usd, fixedNumber }
App->>Comptroller: Get account liquidity (async)
Comptroller->>Market: For each market, get decimals & price
Market->>Market: Per-market decimal detection
Comptroller->>Comptroller: Aggregate liquidity<br/>across mixed decimals
Comptroller-->>App: { liquidity, shortfall }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tropykus/test/deprecation.spec.js (1)
247-259: Assertion contradicts the comment and test intent.The comment states "No new warnings" but the assertion now allows for additional warnings by using
greaterThanOrEqual. This change could mask issues where extra warnings are incorrectly triggered during market operations.If additional warnings are expected in this scenario, update the comment to reflect this. Otherwise, restore the original
.to.equal(initialCallCount)assertion to enforce that no new warnings are emitted.Apply this diff to restore the original strict assertion:
- expect(consoleWarnStub.callCount).to.be.greaterThanOrEqual(initialCallCount); // No new warnings + expect(consoleWarnStub.callCount).to.equal(initialCallCount); // No new warningsOr, if additional warnings are expected, update the comment:
- expect(consoleWarnStub.callCount).to.be.greaterThanOrEqual(initialCallCount); // No new warnings + expect(consoleWarnStub.callCount).to.be.greaterThanOrEqual(initialCallCount); // May have additional warnings in extended flows
🧹 Nitpick comments (16)
packages/tropykus/test/quickstart-validation.spec.js (1)
32-36: Consider consolidating validation or making string checks more flexible.The test duplicates validation already performed in the before hook (lines 27-29 ensure
quickstartContentis a non-empty string). Additionally, checking for the exact string "Deprecate Delisted Markets" may be brittle if documentation wording evolves.Consider either:
- Removing the redundant checks (lines 33-34) since the before hook guarantees them, or
- Using a more flexible pattern match like
/deprecat.*delist/iif only semantic presence matterspackages/tropykus/src/index.js (1)
136-157: ConfirminitialExchangeRatescaling vs contract expectations and consider reusing a shared decimals helper.The new logic to fetch underlying token decimals and then compute:
underlyingDecimals = await erc20Contract.callStatic.decimals(); // ... ethers.utils.parseUnits(args.initialExchangeRate.toString(), underlyingDecimals)is crucial for 6‑decimal tokens, but there are two points worth double‑checking:
Exchange rate mantissa units:
The comment says “Exchange rate is always in 18 decimals in the contract”, but here the mantissa is scaled withunderlyingDecimals. For typical CErc20/CToken patterns, the initial exchange rate mantissa has its own fixed scaling (often 18 or18 + underlyingDecimals - 8). If your CErc20/CRDOC implementation still expects that style of mantissa, scaling by justunderlyingDecimalswill be off by several orders of magnitude for non‑18‑dec tokens. Please re‑verify the constructor signature and docs forCErc20Immutable/CRDOCand ensure the exponent passed intoparseUnitsexactly matches what the contract expects.
If the contract was intentionally changed to expect a mantissa inunderlyingDecimals, consider updating the comment to avoid the “always 18 decimals” wording.Reuse of decimal utilities:
The PR adds decimal utilities (utils/decimals.js) and broader dynamic‑decimal handling in Markets/Comptroller. To avoid duplicating thedecimals()call + try/catch + defaulting to 18 logic here, it may be cleaner to route this through the same shared helper (if one already exists) so behavior and fallbacks stay consistent across the codebase.packages/tropykus/test/price-oracle.spec.js (2)
110-124: Unused helper function.The
deployMockTokenhelper is defined butusdt0Tokendeployed in line 355 is never used in any assertions. Consider either removing the unused variable or adding assertions that validate the token properties.beforeEach(async () => { - // Deploy USDT0 mock token (6 decimals like USDT) - usdt0Token = await deployMockToken('USDT0', 'USDT0', 6); - // Deploy a mock cToken (market) for USDT0
222-229: Brittle error message assertion.The test checks that the error message includes
'DECIMAL_MULTIPLIER', but error messages may vary across ethers.js versions or node implementations. Consider checking for a more general error condition or just verifying that an error was thrown.try { await mocAdapterContract.callStatic.DECIMAL_MULTIPLIER(); expect.fail('DECIMAL_MULTIPLIER should not exist on MoC adapter'); } catch (error) { // Expected - MoC adapter doesn't have DECIMAL_MULTIPLIER - expect(error.message).to.include('DECIMAL_MULTIPLIER'); + // Error is expected when calling non-existent function + expect(error).to.exist; }packages/tropykus/test/01-comptroller.spec.js (2)
29-31: Redundant variable assignment.
crbtcAddressis assigned the same value askrbtcAddressand the comment indicates they're the same market. Consider usingkrbtcAddressdirectly where needed to reduce confusion.const csatAddress = '0xd2ec53e8dd00d204d3d9313af5474eb9f5188ef6'; // kSAT/cSAT (deprecated, mainnet) -const crbtcAddress = krbtcAddress; // kRBTC (use the same as krbtcAddress since it's the active kRBTC market)
58-59: Test assertion is too permissive.Expecting "at least 0 markets" will always pass, even if there's a bug that returns an empty array when markets should exist. Since you're forking mainnet, there should be a known minimum number of markets.
- // On Rootstock Mainnet, there should be at least some markets - expect(markets.length).to.be.at.least(0); + // On Rootstock Mainnet, there should be at least some markets + // Based on the README, there are active markets on mainnet + expect(markets.length).to.be.at.least(1);packages/tropykus/src/Markets/CErc20.js (3)
82-91: Unused private method.
_getTokenFactor()is defined but never called anywhere in this file. Consider removing it or using it in methods likebalanceOfUnderlyingInWalletwhere similar logic is duplicated.Either remove the unused method:
- /** - * Get the token factor (10^tokenDecimals) as a FixedNumber - * @private - * @returns {Promise<FixedNumber>} The token factor - */ - async _getTokenFactor() { - const decimals = await this._ensureDecimals(); - const tokenFactorValue = BigNumber.from(10).pow(decimals).toString(); - return FixedNumber.fromString(tokenFactorValue, format); - }Or use it in
balanceOfUnderlyingInWalletto reduce duplication:- const tokenFactorValue = BigNumber.from(10).pow(decimals).toString(); - ... - const tokenFactor = FixedNumber.fromString(tokenFactorValue, format); + const tokenFactor = await this._getTokenFactor();
24-28: Constructor initiates async operation without error handling.The constructor kicks off
_initializeDecimals()but doesn't handle potential errors. If the async operation fails, the rejection will be unhandled until_ensureDecimals()is called. Consider adding error handling to prevent unhandled promise rejections during construction.this.tokenDecimals = null; // Will be initialized async in _initializeDecimals() - this._decimalsPromise = this._initializeDecimals(); + this._decimalsPromise = this._initializeDecimals().catch((error) => { + console.warn(`[CErc20] Failed to initialize decimals: ${error.message}, will retry on demand`); + this.tokenDecimals = null; // Reset to trigger retry + return 18; // Return default + });
333-344: Minor:redeemwithmaxValue=falseshould validate amount.When
maxValueis false,amountcould benullorundefined(as shown in the exampleawait cusdt.redeem(account, null, true)). If someone accidentally callsredeem(account, null, false),parseTokenAmountwill receive"null"as a string, causing unexpected behavior.async redeem(account, amount, maxValue = false) { if (maxValue) { const kTokens = await this.instance.callStatic.balanceOf(account.address); return this.instance.connect(account.signer) .redeem(kTokens, { gasLimit: this.tropykus.gasLimit }); } + if (amount == null || amount <= 0) { + throw new Error('Amount must be a positive number when maxValue is false'); + } const decimals = await this._ensureDecimals(); const parsedAmount = parseTokenAmount(amount.toString(), decimals);packages/tropykus/test/02-markets.spec.js (3)
292-297: USDT token deployed with 18 decimals instead of 6.The
usdtTokenis deployed with 18 decimals (line 295), but the variable name and context suggest it should represent a USDT-like token which typically has 6 decimals. This may cause confusion and doesn't align with the PR's goal of testing 6-decimal token handling. Consider changing to 6 decimals for consistency, or rename the variable to clarify it's an 18-decimal test token.usdtToken = await usdtTokenFactory.deploy( - ethers.utils.parseEther('1000000'), // 1M tokens + ethers.utils.parseUnits('1000000', 6), // 1M tokens with 6 decimals 'USDT Token', - 18, + 6, // 6 decimals like real USDT 'USDT', );
606-621: afterEach cleanup sets variables to null but doesn't await pending transactions.Setting variables to null in
afterEachdoesn't prevent test interference if there are pending blockchain transactions from the previous test. Consider adding a mechanism to wait for transaction confirmations or relying on Anvil's snapshot/revert functionality for proper test isolation.
1884-1891: Test contains TDD placeholder logic that may always pass.The conditional check
if (cusdt0.tokenDecimals !== undefined)creates a test that passes regardless of whether the feature is implemented. IftokenDecimalsis undefined, the test falls back to checkingerc20Instance.decimals()which tests the underlying token, not the market's decimal detection. This may mask missing functionality.Consider using a stricter assertion or marking the test as pending until the feature is fully implemented.
packages/tropykus/src/Comptroller.js (2)
63-75: Silent fallback when underlying() call fails may hide issues.Creating a CToken with
nullunderlying address whenunderlying()fails could lead to unexpected behavior downstream. The comment mentions "deprecated markets," but this catch-all approach might mask other errors (e.g., network issues, incorrect ABI). Consider logging a warning or differentiating between expected failures (deprecated markets) and unexpected errors.} catch (error) { - // If underlying() call fails (e.g., market is deprecated), still create a CToken - // but without underlying address - this handles deprecated markets gracefully + // If underlying() call fails (e.g., market is deprecated or is CRBTC), + // log a warning and create a CToken without underlying address + console.warn(`Could not fetch underlying for market ${marketAddress}: ${error.message}`); return new CToken(this.tropykus, marketAddress, null); }
367-390: Nested try-catch blocks are redundant.The outer try-catch (lines 367-390) catches errors from creating the market contract, but the inner try-catch (lines 375-386) already handles errors from the
underlying()call. The outer catch setstokenDecimals = 18, which is the same default used throughout. This nesting adds complexity without benefit.- try { - // Check if marketAddress is a CRBTC market (native currency, always 18 decimals) - // For ERC20 markets, we need to get the underlying token address and check decimals - const marketContract = new ethers.Contract( - marketAddress, - CErc20ImmutableArtifact.abi, - this.tropykus.provider, - ); - try { - const underlyingAddress = await marketContract.callStatic.underlying(); - const underlyingContract = new ethers.Contract( - underlyingAddress, - ['function decimals() view returns (uint8)'], - this.tropykus.provider, - ); - tokenDecimals = await underlyingContract.callStatic.decimals(); - } catch (error) { - // If underlying() fails (e.g., CRBTC market), default to 18 decimals - tokenDecimals = 18; - } - } catch (error) { - // If market contract creation fails, default to 18 decimals - tokenDecimals = 18; - } + // Check if marketAddress is a CRBTC market (native currency, always 18 decimals) + // For ERC20 markets, we need to get the underlying token address and check decimals + const marketContract = new ethers.Contract( + marketAddress, + CErc20ImmutableArtifact.abi, + this.tropykus.provider, + ); + try { + const underlyingAddress = await marketContract.callStatic.underlying(); + const underlyingContract = new ethers.Contract( + underlyingAddress, + ['function decimals() view returns (uint8)'], + this.tropykus.provider, + ); + tokenDecimals = await underlyingContract.callStatic.decimals(); + } catch (error) { + // If underlying() fails (e.g., CRBTC market), default to 18 decimals + tokenDecimals = 18; + }packages/tropykus/src/Market.js (2)
509-536:balanceOfUnderlyingInWalletdoes not use dynamic decimal detection.This method uses a hardcoded
factor(1e18) for both the balance and price, unlike the refactored methods that detect token and oracle decimals. For CRBTC (native currency), this is correct, but if this method is called for ERC20 markets, it will produce incorrect results.Looking at the method, it uses
this.tropykus.provider.getBalance()which is specifically for native currency balance, so this appears intentional for CRBTC. However, the method documentation doesn't clarify this limitation. Consider adding a note or creating an override in CErc20.
1006-1016: Potential precision loss when rounding underlying to token decimals.The
toFixed(tokenDecimals)call converts a floating-point number to a string with fixed decimal places, which can introduce rounding errors before parsing. For very large or very small amounts, this may cause precision issues.Consider using a BigNumber-based approach throughout to avoid floating-point conversion:
- const underlyingNumber = Number(underlying._value); - const underlyingRounded = underlyingNumber.toFixed(tokenDecimals); - const underlyingAmountParsed = parseTokenAmount(underlyingRounded, tokenDecimals); + // Use FixedNumber to maintain precision during rounding + const underlyingAmountParsed = parseTokenAmount(underlying._value, tokenDecimals);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
.gitignore(1 hunks)CHANGELOG.md(1 hunks)README.md(3 hunks)lerna.json(1 hunks)package.json(1 hunks)packages/tropykus/artifacts/MockPriceOracleAdapterUSDT.json(1 hunks)packages/tropykus/artifacts/MockPriceProviderMoC.json(1 hunks)packages/tropykus/artifacts/PriceOracleAdapterMoc.json(1 hunks)packages/tropykus/artifacts/PriceOracleAdapterUSDT.json(1 hunks)packages/tropykus/package.json(1 hunks)packages/tropykus/src/Comptroller.js(3 hunks)packages/tropykus/src/Market.js(5 hunks)packages/tropykus/src/Markets/CErc20.js(4 hunks)packages/tropykus/src/PriceOracle.js(2 hunks)packages/tropykus/src/deprecation-config.js(1 hunks)packages/tropykus/src/index.js(2 hunks)packages/tropykus/src/utils/decimals.js(1 hunks)packages/tropykus/test/00-tropykus.spec.js(1 hunks)packages/tropykus/test/01-comptroller.spec.js(2 hunks)packages/tropykus/test/02-markets.spec.js(35 hunks)packages/tropykus/test/03-unitroller.spec.js(2 hunks)packages/tropykus/test/deprecation.spec.js(1 hunks)packages/tropykus/test/price-oracle.spec.js(1 hunks)packages/tropykus/test/quickstart-validation.spec.js(1 hunks)packages/tropykus/test/utils/decimals.spec.js(1 hunks)packages/tropykus/test/utils/deprecation.spec.js(5 hunks)specs/001-deprecate-delisted-markets/checklists/requirements.md(0 hunks)specs/001-deprecate-delisted-markets/contracts/deprecation-api.md(0 hunks)specs/001-deprecate-delisted-markets/contracts/deprecation-config.json(0 hunks)specs/001-deprecate-delisted-markets/data-model.md(0 hunks)specs/001-deprecate-delisted-markets/plan.md(0 hunks)specs/001-deprecate-delisted-markets/quickstart.md(0 hunks)specs/001-deprecate-delisted-markets/research.md(0 hunks)specs/001-deprecate-delisted-markets/spec.md(0 hunks)specs/001-deprecate-delisted-markets/tasks.md(0 hunks)
💤 Files with no reviewable changes (9)
- specs/001-deprecate-delisted-markets/quickstart.md
- specs/001-deprecate-delisted-markets/tasks.md
- specs/001-deprecate-delisted-markets/plan.md
- specs/001-deprecate-delisted-markets/data-model.md
- specs/001-deprecate-delisted-markets/checklists/requirements.md
- specs/001-deprecate-delisted-markets/contracts/deprecation-api.md
- specs/001-deprecate-delisted-markets/research.md
- specs/001-deprecate-delisted-markets/spec.md
- specs/001-deprecate-delisted-markets/contracts/deprecation-config.json
🧰 Additional context used
🧬 Code graph analysis (4)
packages/tropykus/test/00-tropykus.spec.js (2)
packages/tropykus/src/index.js (1)
Tropykus(16-244)packages/tropykus/src/Unitroller.js (1)
Unitroller(4-58)
packages/tropykus/test/03-unitroller.spec.js (1)
packages/tropykus/src/Unitroller.js (1)
Unitroller(4-58)
packages/tropykus/test/utils/decimals.spec.js (1)
packages/tropykus/src/utils/decimals.js (4)
decimals(26-26)getTokenDecimals(15-49)parseTokenAmount(67-69)formatTokenAmount(87-94)
packages/tropykus/src/Markets/CErc20.js (1)
packages/tropykus/src/utils/decimals.js (3)
getTokenDecimals(15-49)decimals(26-26)parseTokenAmount(67-69)
🪛 markdownlint-cli2 (0.18.1)
README.md
40-40: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🔇 Additional comments (33)
.gitignore (1)
497-499: Standard macOS system file entry—LGTM.Adding
.DS_Storeto the ignore list is a widely adopted best practice for macOS development environments.packages/tropykus/test/quickstart-validation.spec.js (1)
14-30: LGTM! Good defensive checks.The enhanced error handling in the before hook properly validates file existence, readability, and content before running tests. The descriptive error messages will help diagnose issues quickly.
lerna.json (1)
2-2: Version metadata bump looks consistent with the 0.4.0 release.
lerna.jsonversion aligns with the root and package versions and the new 0.4.0 changelog entry.package.json (1)
3-3: Root package version correctly aligned to 0.4.0.Matches
lerna.jsonand the documented 0.4.0 release; no functional impact.packages/tropykus/package.json (1)
3-3: Library package.json version correctly bumped to 0.4.0.Keeps the published package version consistent with the monorepo and changelog.
CHANGELOG.md (1)
8-37: 0.4.0 changelog entry is clear and well-aligned with the implementation.The Added/Changed/Documentation/Notes sections accurately describe the new 6‑decimal support, dynamic decimal handling, oracle decimal detection, Anvil test setup, and backward‑compatibility guarantees. No issues from a changelog/semver perspective.
packages/tropykus/artifacts/PriceOracleAdapterUSDT.json (1)
1-162: PriceOracleAdapterUSDT artifact structure and ABI look consistent for deployment/use.The artifact includes the expected constructor, guardian management, price provider wiring,
DECIMAL_MULTIPLIER, andassetPricesview, plus bytecode/deployedBytecode and link references, matching how other Hardhat artifacts in the repo are typically shaped. No issues spotted here.packages/tropykus/artifacts/MockPriceOracleAdapterUSDT.json (1)
1-162: LGTM!This artifact file is properly structured and contains the expected components (ABI, bytecode, deployedBytecode, linkReferences). Artifact files are typically auto-generated by build tools.
packages/tropykus/artifacts/PriceOracleAdapterMoc.json (1)
1-147: LGTM!This artifact file is properly structured with all required components. Artifact files are auto-generated by the build process.
packages/tropykus/test/utils/deprecation.spec.js (1)
8-11: LGTM!The address constants have been updated consistently throughout the test file. The test logic remains intact with only the test data (addresses) being updated.
packages/tropykus/src/utils/decimals.js (3)
15-49: Good error handling with appropriate fallbacks.The function properly handles various edge cases including null inputs, missing decimals() function, BigNumber/string return types, and invalid decimal values. The fallback to 18 decimals is standard for ERC20 tokens.
67-69: LGTM!Simple and effective wrapper around ethers.utils.parseUnits. The toString() call ensures both string and numeric inputs are handled correctly.
87-94: LGTM!The function correctly handles both BigNumber and string inputs by checking the type before conversion. The use of ethers.utils.formatUnits ensures proper formatting.
packages/tropykus/test/03-unitroller.spec.js (2)
29-63: Excellent test refactoring.Deploying fresh contracts avoids permission issues on forked networks and makes the test more reliable. The test properly:
- Deploys fresh Unitroller and Comptroller instances
- Verifies initial state before changes
- Waits for transaction confirmation
- Validates the final state
65-88: LGTM!The test correctly uses tropykus.setComptroller to deploy and configure a new Comptroller implementation, then verifies it was set as the active implementation. The fresh Unitroller deployment ensures the test is self-contained.
packages/tropykus/test/utils/decimals.spec.js (1)
1-316: Excellent test coverage!This test suite comprehensively covers all three decimal utility functions with:
- Multiple decimal configurations (0, 2, 6, 8, 18)
- Error handling and fallback scenarios
- Type handling (BigNumber, string, number)
- Edge cases (very small/large amounts, zero values)
- Integration round-trip tests
The use of mocks and stubs is appropriate, and the test structure is clear and maintainable.
packages/tropykus/artifacts/MockPriceProviderMoC.json (1)
1-97: LGTM!This artifact file is properly structured with all required components. Artifact files are auto-generated by the build process.
packages/tropykus/test/00-tropykus.spec.js (2)
22-26: LGTM!Good update to accept multiple chain IDs for cross-network test compatibility. The conversion to
Number()handles BigNumber returns correctly.
43-71: LGTM!Deploying a fresh Unitroller for testing is the correct approach to avoid permission issues when forking mainnet. The test properly validates the proxy-implementation relationship.
packages/tropykus/src/PriceOracle.js (1)
35-85: LGTM!The decimal detection logic is well-structured:
- Null/undefined handling with fallback to 18 decimals
- Caching with normalized lowercase addresses prevents duplicate queries
- DECIMAL_MULTIPLIER probe correctly distinguishes USDT (30 decimals) from MoC (18 decimals) adapters
- Error handling gracefully falls back to 18 decimals for unknown adapters
packages/tropykus/test/price-oracle.spec.js (1)
132-347: LGTM!Comprehensive test coverage for
detectOracleDecimals:
- Tests both USDT (30 decimals) and MoC (18 decimals) adapters
- Validates caching behavior and address normalization
- Handles null/undefined inputs gracefully
- Verifies DECIMAL_MULTIPLIER detection logic
packages/tropykus/test/01-comptroller.spec.js (1)
76-110: LGTM!Good pattern for testing in a forked environment — deploying a fresh Unitroller avoids permission issues with the mainnet admin. The conditional logic handles both empty and populated market scenarios correctly.
packages/tropykus/src/Markets/CErc20.js (2)
152-178: LGTM!Good fix for the delta calculation in
repayBorrow. Using dynamic token decimals instead of hardcoded1e18ensures correct behavior for non-18-decimal tokens like USDT (6 decimals).
262-308: LGTM!Well-implemented dynamic decimal handling for both token and oracle precision. Using
Promise.allfor parallel fetching of balance and price is efficient. The FixedNumber calculations maintain precision throughout.packages/tropykus/test/02-markets.spec.js (1)
2393-2404: LGTM - Comprehensive liquidity assertions across multiple markets.The test correctly verifies account liquidity across markets with different decimal precisions (6 for USDT0, 18 for DOC and RBTC). The assertions properly validate both USD and underlying values with appropriate tolerance using
closeTo().packages/tropykus/src/Comptroller.js (2)
268-341: LGTM - Well-structured market-specific liquidity calculation with proper decimal handling.The implementation correctly:
- Detects token decimals from underlying contract with fallback to 18 for CRBTC
- Detects oracle decimals from the price oracle adapter
- Uses BigNumber.pow() to avoid scientific notation issues
- Calculates liquidity in both USD and underlying terms with proper factor conversion
- Processes balance, price, and collateral factor in parallel for efficiency
474-517: LGTM - Efficient parallel processing with proper decimal-aware calculations.The
getTotalBorrowsInAllMarketsimplementation correctly:
- Processes markets in parallel using
Promise.all- Detects both token and oracle decimals per-market
- Converts borrows to human-readable format before USD calculation
- Properly handles the target market price for underlying conversion
packages/tropykus/src/Market.js (6)
54-102: LGTM - Proper dynamic decimal detection for balance calculations.The
balanceOfUnderlyingmethod correctly:
- Detects token decimals via
_ensureDecimals()or falls back totokenDecimalsproperty- Detects oracle decimals from the price adapter
- Uses BigNumber.pow() for factor calculation to avoid scientific notation
- Performs parallel contract calls for efficiency
- Converts both balance and price to human-readable before USD calculation
314-332:redeemmethod usesparseEtherforredeemUnderlying.The
redeemUnderlyingcall at line 325 usesethers.utils.parseEther(amount.toString())which assumes 18 decimals. For tokens with different decimal precisions, this will cause incorrect redemption amounts. Verify CErc20 override handles this correctly.
756-805: LGTM - Correct handling of cToken vs underlying decimal separation.The
getMarketTotalSupplymethod correctly distinguishes:
- cToken totalSupply: Always 18 decimals (divided by
cTokenFactor)- Exchange rate: Always 18 decimals (divided by
cTokenFactor)- Oracle price: Uses detected oracle decimals
- The underlying calculation correctly multiplies cTokens × exchangeRate
1213-1253: LGTM - Event-based supply aggregation with proper decimal handling.The
suppliedLast24Hoursmethod correctly:
- Detects token and oracle decimals
- Aggregates mint amounts from events
- Converts from token decimals to human-readable
- Calculates USD value using properly converted price
294-304: CErc20 class properly overridesborrow()with decimal-aware parsing — no issue.The CErc20 class at line 201-207 overrides the
borrow()method and usesparseTokenAmount(amount.toString(), decimals)after retrieving the actual token decimals viathis._ensureDecimals(). This correctly handles both 6-decimal tokens (like CUSDT) and 18-decimal tokens (like CDOC), so the base Market class's use ofparseEtheris safely overridden in subclasses that need it.
278-286: CErc20 correctly overridesmint()with proper decimal handling — no changes needed.The Market.js
mintmethod correctly usesparseEtherfor CRBTC (native currency, always 18 decimals). CErc20 properly overrides this method withparseTokenAmount(amount.toString(), decimals), which callsethers.utils.parseUnitswith the token's actual decimals fetched from the contract via_ensureDecimals(). This correctly supports 6-decimal tokens like USDT and other non-18-decimal tokens.
| if (market.address === marketAddress.toLowerCase()) { | ||
| priceUnderlying = priceHumanReadable; | ||
| } |
There was a problem hiding this comment.
Comparison uses strict equality on non-normalized address.
Line 605 compares market.address with marketAddress.toLowerCase(), but market.address may not be lowercase. This could cause the condition to never match, resulting in priceUnderlying remaining at 0 and incorrect underlying calculations.
- if (market.address === marketAddress.toLowerCase()) {
+ if (market.address.toLowerCase() === marketAddress.toLowerCase()) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (market.address === marketAddress.toLowerCase()) { | |
| priceUnderlying = priceHumanReadable; | |
| } | |
| if (market.address.toLowerCase() === marketAddress.toLowerCase()) { | |
| priceUnderlying = priceHumanReadable; | |
| } |
🤖 Prompt for AI Agents
In packages/tropykus/src/Comptroller.js around lines 605 to 607, the comparison
uses strict equality between market.address and marketAddress.toLowerCase()
while market.address may not be lowercased; normalize both sides before
comparing (e.g., ensure market.address exists then compare
market.address.toLowerCase() === marketAddress.toLowerCase()) so the match
succeeds and priceUnderlying is set correctly.
There was a problem hiding this comment.
@NataliaMunozC, you're absolutely right—this is important!
The issue identified by CodeRabbit is a real bug: on line 605 in getTotalSupplyInAllMarkets, the comparison market.address === marketAddress.toLowerCase() will fail if market.address isn't already lowercase. This means priceUnderlying would remain 0, leading to incorrect underlying value calculations when a specific marketAddress is provided.
Interestingly, this same comparison is done correctly earlier in the file: In getTotalBorrowsInAllMarkets (around line 519 in the diff), the code properly normalizes both sides:
if (market.address.toLowerCase() === marketAddress.toLowerCase()) {So there's an inconsistency between these two similar methods. The fix is straightforward—apply the same normalization in getTotalSupplyInAllMarkets:
if (market.address.toLowerCase() === marketAddress.toLowerCase()) {
priceUnderlying = priceHumanReadable;
}This will ensure both methods handle address comparisons consistently and correctly. I'd recommend applying CodeRabbit's suggestion to fix this.
| // kRDOC/cRDOC market address - deprecated because market was delisted from protocol | ||
| '0x0000000000000000000000000000000000000000': { | ||
| deprecated: true, | ||
| reason: 'Market never listed (kRDOC/cRDOC)', | ||
| reason: 'Market delisted from protocol (kRDOC/cRDOC)', | ||
| }, |
There was a problem hiding this comment.
kRDOC/cRDOC deprecation reason is inconsistent with existing docs.
Here the reason is “Market delisted from protocol (kRDOC/cRDOC)”, but:
- README still describes cRDOC as “deprecated (never listed)”.
- The 0.3.0 changelog entry for kRDOC says “Market never listed in the protocol”.
This inconsistency can be confusing for users trying to understand why kRDOC/cRDOC is deprecated. Consider choosing a single canonical reason (“never listed” vs “delisted”) and aligning:
- This config entry
- The README cRDOC deprecation note
- (Optionally) a brief clarification in a newer changelog entry if the status truly changed over time.
🤖 Prompt for AI Agents
packages/tropykus/src/deprecation-config.js lines 23-27: the deprecation reason
for the kRDOC/cRDOC entry currently says "Market delisted from protocol
(kRDOC/cRDOC)" which conflicts with README and the 0.3.0 changelog that state it
was "never listed"; pick a single canonical reason (preferably "never listed" to
match existing docs) and update this config entry to use that canonical
phrasing, then update the README cRDOC deprecation note (and optionally add a
short changelog clarification if the historical status actually changed) so all
three places (this config, README, changelog) are consistent.
| const rawPrice = await this.instance.callStatic.getUnderlyingPrice(marketAddress); | ||
|
|
||
| // Divide by the correct factor based on adapter type to get human-readable price | ||
| // MoC: divide by 1e18, USDT: divide by 1e30 | ||
| const divisor = BigNumber.from(10).pow(oracleDecimals); | ||
| const price = BigNumber.from(rawPrice).div(divisor); | ||
|
|
||
| return Number(price.toString()); |
There was a problem hiding this comment.
Potential precision loss when dividing large BigNumbers.
BigNumber.div() performs integer division, truncating any fractional component. For prices that aren't exact multiples of the divisor, this could lose significant precision. For example, a price of 1.5e30 divided by 1e30 would correctly yield 1, but 1.5e30 doesn't exist as a BigNumber — fractional values are truncated.
Additionally, converting to Number() at line 128 may lose precision for prices with more than 15-16 significant digits.
Consider using FixedNumber for price calculations (consistent with CErc20.js) or returning the raw BigNumber to preserve precision:
- const divisor = BigNumber.from(10).pow(oracleDecimals);
- const price = BigNumber.from(rawPrice).div(divisor);
-
- return Number(price.toString());
+ const divisor = FixedNumber.fromString(
+ BigNumber.from(10).pow(oracleDecimals).toString(),
+ 'fixed80x18'
+ );
+ const price = FixedNumber.from(rawPrice.toString(), 'fixed80x18')
+ .divUnsafe(divisor);
+
+ return Number(price._value);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/tropykus/src/PriceOracle.js around lines 121-128, the current use of
BigNumber.div causes integer truncation and the final Number(...) conversion can
lose precision; replace the integer division with a fixed-point calculation (use
ethers FixedNumber or a decimal library) to divide rawPrice by
10**oracleDecimals with preserved fractional precision and return a string (or
FixedNumber) instead of converting to Number, or alternatively return the raw
BigNumber/decimal string to callers so no precision is lost.
| alice = tropykus.getAccountFromMnemonic(mnemonic, `m/44'/60'/0'/0/1`); | ||
| bob = tropykus.getAccountFromMnemonic(mnemonic, `m/44'/60'/0'/0/2`); | ||
| carlos = tropykus.getAccountFromMnemonic(mnemonic, `m/44'/60'/0'/0/3`); | ||
| david = tropykus.getAccountFromMnemonic(mnemonic, `m/44'/60'/0'/0/4`); | ||
| eve = tropykus.getAccountFromMnemonic(mnemonic, `m/44'/60'/0'/0/4`); |
There was a problem hiding this comment.
Variable eve is assigned the same derivation path as david.
Both david and eve are derived from the same HD path m/44'/60'/0'/0/4, making them the same account. This appears to be a copy-paste error.
david = tropykus.getAccountFromMnemonic(mnemonic, `m/44'/60'/0'/0/4`);
- eve = tropykus.getAccountFromMnemonic(mnemonic, `m/44'/60'/0'/0/4`);
+ eve = tropykus.getAccountFromMnemonic(mnemonic, `m/44'/60'/0'/0/5`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| alice = tropykus.getAccountFromMnemonic(mnemonic, `m/44'/60'/0'/0/1`); | |
| bob = tropykus.getAccountFromMnemonic(mnemonic, `m/44'/60'/0'/0/2`); | |
| carlos = tropykus.getAccountFromMnemonic(mnemonic, `m/44'/60'/0'/0/3`); | |
| david = tropykus.getAccountFromMnemonic(mnemonic, `m/44'/60'/0'/0/4`); | |
| eve = tropykus.getAccountFromMnemonic(mnemonic, `m/44'/60'/0'/0/4`); | |
| alice = tropykus.getAccountFromMnemonic(mnemonic, `m/44'/60'/0'/0/1`); | |
| bob = tropykus.getAccountFromMnemonic(mnemonic, `m/44'/60'/0'/0/2`); | |
| carlos = tropykus.getAccountFromMnemonic(mnemonic, `m/44'/60'/0'/0/3`); | |
| david = tropykus.getAccountFromMnemonic(mnemonic, `m/44'/60'/0'/0/4`); | |
| eve = tropykus.getAccountFromMnemonic(mnemonic, `m/44'/60'/0'/0/5`); |
🤖 Prompt for AI Agents
In packages/tropykus/test/02-markets.spec.js around lines 560 to 564, eve is
created using the same HD derivation path as david (`m/44'/60'/0'/0/4`) causing
both to be identical; change eve's derivation path to the next index (e.g.
`m/44'/60'/0'/0/5`) so eve is a distinct account, and run the tests to confirm
no other references rely on the duplicated path.
| // Clear all variables | ||
| usdt0Token = null; | ||
| cusdt0 = null; | ||
| cdoc = null; | ||
| newComptroller = null; | ||
| alice = null; | ||
| markets = null; | ||
| }); |
There was a problem hiding this comment.
afterEach cleanup is incomplete - missing crbtc and bob cleanup.
The afterEach block in 'Multi-market operations' doesn't set crbtc and bob to null, unlike the 'Markets operations' block which cleans up all variables. This inconsistency could lead to memory leaks or test interference.
afterEach(async () => {
// Clear all variables
usdt0Token = null;
cusdt0 = null;
cdoc = null;
+ crbtc = null;
newComptroller = null;
alice = null;
+ bob = null;
markets = null;
});🤖 Prompt for AI Agents
In packages/tropykus/test/02-markets.spec.js around lines 2370 to 2377, the
afterEach cleanup for the "Multi-market operations" tests omits resetting crbtc
and bob to null; update that afterEach block to also set crbtc = null and bob =
null so all test-scoped variables are cleared consistently to prevent state
leakage between tests.
| ## Running Tests | ||
|
|
||
| Before running the test suite, you need to start a local Anvil node that forks the RSK Mainnet network: | ||
|
|
||
| ```bash | ||
| anvil --fork-url https://public-node.rsk.co --chain-id 30 --port 8545 | ||
| ``` | ||
|
|
||
| This command: | ||
| - Forks the RSK Mainnet network (`https://public-node.rsk.co`) | ||
| - Sets the chain ID to 30 (RSK Mainnet) | ||
| - Runs the local node on port 8545 (default) | ||
|
|
||
| Once Anvil is running, you can run the tests in a separate terminal: | ||
|
|
||
| ```bash | ||
| yarn test | ||
| ``` | ||
|
|
||
| > **Note**: Make sure Anvil is running before executing tests, as the test suite requires a local blockchain node to be available at `http://127.0.0.1:8545`. | ||
|
|
||
| > **⚠️ Important**: Before running the full test suite (`yarn test`), you **MUST** restart the Anvil node to ensure a clean state. This prevents nonce conflicts, state pollution, and transaction errors from previous test runs. To restart Anvil, stop the current process and start a fresh instance with the same command above. | ||
|
|
There was a problem hiding this comment.
Test instructions and USDT0 clarifications read well; just fix the markdownlint blockquote warning.
The new “Running Tests” section and the Anvil notes are clear and match the new Anvil-based test setup, and the USDT0 vs kUSDT decimal explanation later in the README is very helpful.
markdownlint (MD028) is flagging a “Blank line inside blockquote” around the Anvil notes (lines 39–41). There appears to be an empty blockquote line between the two quoted notes. You can fix this by either:
- Removing the empty quoted line entirely, or
- Making sure each blockquote paragraph is consecutive without a blank
>line in between.
Content-wise this section is good; this is just to keep markdownlint happy.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
40-40: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 Prompt for AI Agents
In README.md around lines 20 to 42, markdownlint MD028 flags a blank line inside
a blockquote in the Anvil notes; remove the empty quoted line or merge the two
blockquote paragraphs so there is no `>`-prefixed blank line between them
(ensure consecutive blockquote lines or collapse into a single blockquote
paragraph) to satisfy markdownlint.
There was a problem hiding this comment.
@NataliaMunozC, here's the fix for the markdownlint warning. You need to remove the empty line between the two blockquote notes.
Current code (with the blank line issue):
> **Note**: Make sure Anvil is running before executing tests, as the test suite requires a local blockchain node to be available at `http://127.0.0.1:8545`.
> **⚠️ Important**: Before running the full test suite (`yarn test`), you **MUST** restart the Anvil node to ensure a clean state. This prevents nonce conflicts, state pollution, and transaction errors from previous test runs. To restart Anvil, stop the current process and start a fresh instance with the same command above.Fixed version (remove the empty line between blockquotes):
> **Note**: Make sure Anvil is running before executing tests, as the test suite requires a local blockchain node to be available at `http://127.0.0.1:8545`.
> **⚠️ Important**: Before running the full test suite (`yarn test`), you **MUST** restart the Anvil node to ensure a clean state. This prevents nonce conflicts, state pollution, and transaction errors from previous test runs. To restart Anvil, stop the current process and start a fresh instance with the same command above.Alternatively, you could merge them into a single blockquote with a blank line inside (but this is less clear):
> **Note**: Make sure Anvil is running before executing tests, as the test suite requires a local blockchain node to be available at `http://127.0.0.1:8545`.
>
> **⚠️ Important**: Before running the full test suite (`yarn test`), you **MUST** restart the Anvil node to ensure a clean state. This prevents nonce conflicts, state pollution, and transaction errors from previous test runs. To restart Anvil, stop the current process and start a fresh instance with the same command above.I recommend the first approach (consecutive blockquotes with no empty line between them) as it's cleaner and satisfies markdownlint.
… naming conventions. Change package name from 'tropykusjs' to '@tropykus/tropykuslibs' in package.json, README.md, and related files. Update versioning to '0.4.0-alpha' in lerna.json and package.json. Adjust scripts for publishing and checksum calculations to reflect new package name. Ensure all references are consistent across the project.
…cToken references and add USDT0 market details in README. Include new .claude directory in .gitignore. Remove old README.md from tropykus package.
… publish script to determine npm tag based on version type, supporting prerelease tags (alpha, beta, rc) for better package management.
2 participants
Release 0.4.0: 6-Decimal Token Support and Dynamic Decimal Handling
Summary
This release introduces comprehensive support for 6-decimal ERC20 tokens (including USDT0) and implements dynamic decimal detection throughout the Tropykus SDK. The changes ensure accurate handling of tokens with varying decimal precisions while maintaining full backward compatibility with existing 18-decimal tokens.
Key Features
Added
detectOracleDecimalsmethod for automatic oracle adapter decimal detectionChanged
mint,borrow,repayBorrow,redeem,balanceOfUnderlying) to use dynamic decimal handling instead of hardcoded factorscloseTofor better precision in financial calculationsTechnical Details
Decimal Handling
Backward Compatibility
Documentation Updates
Testing
Migration Notes
No migration required for existing integrations. The changes are fully backward compatible. New integrations using 6-decimal tokens (like USDT0) will automatically benefit from the improved decimal handling.
Related Issues
This release addresses the need for proper handling of 6-decimal tokens in the Tropykus protocol, particularly for USDT0 integration.
Summary by CodeRabbit
Release Notes v0.4.0
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.