M-M09: reject asset decimals exceeding its token's decimals

[philanton]

Description

GetAssetDecimals and GetTokenDecimals read from two independent maps in the memory store, populated from separate config fields with no requirement that they are the same. The asset-level field is a single value per asset symbol, while GetTokenDecimals is keyed on blockchainID and tokenAddress, meaning the same asset can have different precision on different chains.
This is also problematic in escrow flows, where the same transition amount may need to be represented on both the home and escrow ledgers. If those two token representations use different decimals, the request can pass transition validation under the asset-level precision and fail later when one of the ledgers is converted using the lower token precision.
Consequently, valid operations can be rejected or accepted only to fail later in the signing flow.

Summary by CodeRabbit

• Bug Fixes
  • Added validation to ensure asset decimal values do not exceed their associated token decimal values. Invalid configurations are now rejected with detailed error messages during asset verification and caching.

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes introduce a new validation rule that ensures asset decimals do not exceed associated token decimals. This validation is enforced in two layers: the asset configuration verification in clearnode and the SDK asset cache population, with corresponding test coverage for both success and failure paths.

Changes

Cohort / File(s)	Summary
Configuration Validation clearnode/store/memory/asset_config.go, clearnode/store/memory/asset_config_test.go	Added decimals validation to verifyAssetsConfig that returns an error if asset decimals exceed token decimals. Includes two new test cases: one verifying the error when asset decimals (8) exceed token decimals (6), and one confirming success when both are equal (6).
SDK Asset Caching sdk/go/asset_cache.go, sdk/go/asset_cache_test.go	Integrated decimals validation into clientAssetStore.populateCache to reject assets before caching if their decimals exceed any associated token's decimals. Added new test case demonstrating the validation error scenario with USDC asset (18 decimals) and token (6 decimals).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested reviewers

• ihsraham
• dimast-x

Poem

🐰 A hop, skip, and decimal check—
Asset and token must align,
No excess zeros for a wreck,
Validation makes them shine!
When decimals dance hand in hand,
The cache holds fortune vast and grand. ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main validation being added: rejecting assets whose decimal precision exceeds their associated token's decimal precision.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/clearnode-m-09

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[nksazonov]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧹 Nitpick comments (1)

clearnode/store/memory/asset_config_test.go (1)

100-123: Consider adding a test case for asset decimals less than token decimals.

The current tests cover the failure case (asset > token) and boundary case (asset == token), but don't explicitly test the valid scenario where asset decimals are less than token decimals.

Based on learnings, the YUSD test token has different decimals across chains (6 on Sepolia, 8 on Amoy), so an asset with Decimals: 6 paired with a token having Decimals: 8 should be valid.

🧪 Suggested additional test case

+	// Test asset decimals less than token decimals (should pass)
+	t.Run("asset decimals less than token decimals", func(t *testing.T) {
+		cfg := AssetsConfig{
+			Assets: []AssetConfig{
+				{
+					Name:                  "USD Coin",
+					Symbol:                "USDC",
+					Decimals:              6,
+					SuggestedBlockchainID: 1,
+					Tokens: []TokenConfig{
+						{
+							Name:         "USD Coin",
+							Symbol:       "USDC",
+							BlockchainID: 1,
+							Address:      "0x2791bca1f2de4661ed88a30c99a7a9449aa84174",
+							Decimals:     8,
+						},
+					},
+				},
+			},
+		}
+		err := verifyAssetsConfig(&cfg)
+		require.NoError(t, err)
+	})
+
 	// Test custom symbol for token (inherits from asset when empty)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@clearnode/store/memory/asset_config_test.go` around lines 100 - 123, Add a
new test in asset_config_test.go that verifies the valid scenario where an
asset's Decimals is less than its token's Decimals: create an AssetsConfig with
one AssetConfig (e.g., Decimals: 6) containing a TokenConfig with higher
Decimals (e.g., 8), call verifyAssetsConfig(&cfg) and assert require.NoError(t,
err); place the test alongside the existing cases and name it "asset decimals
less than token decimals" to target the verifyAssetsConfig validation logic.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@clearnode/store/memory/asset_config_test.go`:
- Around line 100-123: Add a new test in asset_config_test.go that verifies the
valid scenario where an asset's Decimals is less than its token's Decimals:
create an AssetsConfig with one AssetConfig (e.g., Decimals: 6) containing a
TokenConfig with higher Decimals (e.g., 8), call verifyAssetsConfig(&cfg) and
assert require.NoError(t, err); place the test alongside the existing cases and
name it "asset decimals less than token decimals" to target the
verifyAssetsConfig validation logic.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 857d3d39-b347-4d81-bf59-891a6a6e82c5

📥 Commits

Reviewing files that changed from the base of the PR and between 38d2ad1 and 957b1ea.

📒 Files selected for processing (4)

• clearnode/store/memory/asset_config.go
• clearnode/store/memory/asset_config_test.go
• sdk/go/asset_cache.go
• sdk/go/asset_cache_test.go