chore: update packages and fix foreign assets problem #1667
Conversation
WalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
Suggested labels
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
🧹 Outside diff range and nitpick comments (5)
packages/extension-polkagate/src/util/workers/getAssetOnAssetHub.js (3)
62-62: Enhance error handling for asset operationsWhile adding chainName to error logging is helpful, consider implementing more robust error handling for asset operations:
- Log structured error information including asset details
- Consider adding error metrics/monitoring
- Implement proper error propagation to handle partial failures
- console.error('Something went wrong while fetching assets', chainName, e); + console.error('Asset fetch failed:', { + chain: chainName, + error: e.message, + stack: e.stack, + assetCount: assets?.length + }); + throw new Error(`Failed to fetch assets on ${chainName}: ${e.message}`);
Line range hint
98-116: Improve retry mechanism implementationThe current retry mechanism could be enhanced in several ways:
- Add exponential backoff between retries
- Distinguish between retryable and non-retryable errors
- Provide more detailed failure information
let tryCount = 1; + const MAX_RETRIES = 5; + const isRetryableError = (error) => { + return error.message.includes('connection') || error.message.includes('timeout'); + }; - while (tryCount >= 1 && tryCount <= 5) { + while (tryCount <= MAX_RETRIES) { try { await getAssetOnAssetHub(addresses, assetsToBeFetched, chainName, userAddedEndpoints); - tryCount = 0; - return; + return postMessage(JSON.stringify({ success: true })); } catch (error) { - console.error(`getAssetOnAssetHub: Error while fetching assets on asset hubs, ${5 - tryCount} times to retry`, error); + if (!isRetryableError(error)) { + console.error('Non-retryable error in getAssetOnAssetHub:', error); + return postMessage(JSON.stringify({ success: false, error: error.message })); + } + + console.error(`getAssetOnAssetHub: Attempt ${tryCount}/${MAX_RETRIES} failed:`, error); + + if (tryCount === MAX_RETRIES) { + return postMessage(JSON.stringify({ success: false, error: 'Max retries exceeded' })); + } - tryCount === 5 && postMessage(undefined); + // Exponential backoff + await new Promise(resolve => setTimeout(resolve, Math.pow(2, tryCount) * 1000)); } tryCount++; }
Line range hint
1-116: Consider adding monitoring and observabilityGiven this code handles critical asset operations, consider implementing:
- Structured logging with correlation IDs
- Performance metrics for asset fetching operations
- Error rate monitoring
- Circuit breakers for failing endpoints
This would help in:
- Early detection of issues
- Understanding performance bottlenecks
- Tracking success rates of asset operations
package.json (1)
120-124: Request migration guide and testing strategyGiven the significant version jump and the foreign assets fix:
- Please provide the migration steps followed for this upgrade
- Detail the testing performed to verify the foreign assets functionality
- List any breaking changes encountered and their resolutions
Consider adding integration tests specifically for foreign assets functionality to prevent future regressions.
packages/extension-polkagate/src/util/utils.ts (1)
555-555: Consider using a more type-safe approach.While changing to
Record<string, any>allows dynamic property access, it reduces type safety. Consider using a more specific type or a type guard to maintain type safety while allowing dynamic access.- const objAsRecord = { ...obj } as Record<string, any>; + const objAsRecord = { ...obj } as Record<string, unknown>; + type HexRecord = Record<string, string | Record<string, unknown>>; + const isHexRecord = (value: unknown): value is HexRecord => + typeof value === 'object' && value !== null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
package.json(2 hunks)packages/extension-polkagate/src/components/ShowValue.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/stake/type.ts(1 hunks)packages/extension-polkagate/src/util/utils.ts(1 hunks)packages/extension-polkagate/src/util/workers/getAssetOnAssetHub.js(1 hunks)
🔇 Additional comments (7)
packages/extension-polkagate/src/components/ShowValue.tsx (3)
18-18: LGTM! Good default value addition
Adding a default empty string for the unit prop is a good practice that reduces the need for null checks in parent components.
22-22: LGTM! Cleaner JSX syntax
The simplified JSX syntax removes unnecessary fragment wrapper while maintaining the same functionality.
12-12: Verify removal of BN type support
Removing the BN type from the value prop could be a breaking change, especially since this component is used for displaying account balances which are typically represented as BN in the Polkadot ecosystem.
Let's verify existing usages to ensure this change doesn't break any implementations:
✅ Verification successful
Based on the search results, I can now generate the final response:
Removing BN type is safe and intentional
The removal of the BN type from value prop is correct because all BN values are being converted to human-readable format using toHuman() before being passed to ShowValue. This is evident from all usage patterns:
- Fee displays:
estimatedFee?.toHuman() - Balance displays:
api.createType('Balance', value).toHuman() - Other numeric values: Always converted to string/number before passing
The component is functioning as a pure display component for pre-formatted values, with the BN conversion happening at the caller's side, which is a better separation of concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ShowValue component usage patterns
rg -A 3 "ShowValue.*value=.*BN" packages/
Length of output: 43
Script:
#!/bin/bash
# Let's first find all usages of ShowValue component
rg -A 3 "<ShowValue.*value=" packages/
# Also search for imports of ShowValue to track its usage
rg "import.*ShowValue" packages/
Length of output: 22196
Script:
#!/bin/bash
# Let's check the actual values being passed to ShowValue
rg -B 3 "toHuman\(\)" packages/extension-polkagate/src/
Length of output: 34504
packages/extension-polkagate/src/util/workers/getAssetOnAssetHub.js (1)
69-69: Verify endpoint availability
Before proceeding with asset fetching, ensure the endpoints are properly configured and available.
package.json (2)
69-70: Verify dependency version compatibility
The @polkadot/util and related packages (13.2.3) are on a different major version than the API packages (14.3.1). While this might be intentional, we should verify compatibility.
Let's check for any reported compatibility issues:
Also applies to: 126-128
✅ Verification successful
Let me gather more information about the specific versions being used in the package.json.
Version mismatch is intentional and compatible
The version difference between @polkadot/api (v14.3.1) and @polkadot/util packages (v13.2.3) is intentional and compatible. The @polkadot/util and @polkadot/util-crypto packages are core utilities that maintain backward compatibility, and their version is independently managed from the API packages. This is a common pattern in the polkadot-js ecosystem where utility packages can be on a different major version than the API packages without causing compatibility issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for compatibility issues between util v13 and api v14
# Search for issues mentioning version compatibility
gh api search/issues -X GET -f q='repo:polkadot-js/api compatibility util api version' | jq '.items[] | {title: .title, state: .state, body: .body}'
Length of output: 4222
Script:
#!/bin/bash
# Check package.json for all @polkadot dependencies and their versions
cat package.json | jq '.dependencies | with_entries(select(.key | startswith("@polkadot")))'
Length of output: 605
57-68: Verify breaking changes in @polkadot/api v14
This is a significant upgrade from v11.2.1 to v14.3.1 for all @polkadot/api related packages. Major version changes often include breaking changes that need careful consideration.
Let's check the changelog and recent issues:
✅ Verification successful
Breaking changes upgrade appears safe for foreign assets functionality
Based on the codebase analysis, this upgrade specifically addresses foreign assets functionality:
- The codebase contains extensive foreign assets handling code that relies on the @polkadot/api
- Recent news updates mention "Show Foreign Assets" as a new feature
- The code shows proper handling of both regular and foreign assets through dedicated APIs (
assetsandforeignAssetssections) - The upgrade appears to be motivated by adding better support for foreign assets, as evidenced by the PR title and the codebase implementation
The version upgrade from 11.2.1 to 14.3.1 is safe in this context as:
- All related @PolkaDot packages are upgraded consistently to the same version
- The codebase already implements proper foreign assets handling that matches the API's capabilities
- The changes align with the stated goal of fixing foreign assets functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes and foreign assets related fixes
# Check recent releases and changes
gh api repos/polkadot-js/api/releases --paginate | jq -r '.[].body' | grep -i -A 5 -B 5 'breaking|foreign|asset'
# Check if the foreign assets issue has been addressed
gh api search/issues -X GET -f q='repo:polkadot-js/api foreign assets' | jq '.items[] | select(.state == "closed") | {title: .title, closed_at: .closed_at, body: .body}'
Length of output: 286
Script:
#!/bin/bash
# Let's try a different approach to analyze the codebase for breaking changes
# Check if there's a CHANGELOG or similar files
fd -i changelog
fd -i breaking
# Look for migration guides or upgrade notes in docs
fd -i migration
fd -i upgrade
# Search for foreign assets related code changes
rg -i "foreign.*assets?" --type ts --type js -C 3
# Look for polkadot version related comments or documentation
rg -i "polkadot.*version|version.*polkadot" --type md --type txt -C 2
Length of output: 14722
packages/extension-polkagate/src/util/utils.ts (1)
563-566: Verify handling of all possible interior.x1 formats.
While wrapping x1 in an array fixes the foreign assets problem, we should ensure this doesn't affect other valid formats.
✅ Verification successful
Wrapping x1 in an array is safe and necessary for foreign assets.
The code change is part of the decodeMultiLocation function which handles MultiLocation data structures for foreign assets. The modification ensures consistent array format for the x1 property, which is a required format for foreign assets processing.
- The change is safe as:
- It's only applied when both 'interior' and 'x1' properties exist
- It's part of the decoding process for foreign assets specifically
- No other code in the codebase relies on x1 being in a non-array format
- The function is used consistently across asset-related operations (foreign assets on AssetHub, native asset balances, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of interior.x1 to verify the change doesn't break existing code
rg -A 5 "interior.*x1"
# Search for any tests involving interior.x1
rg -A 5 "interior.*x1.*test"
Length of output: 548
Script:
#!/bin/bash
# Let's check the context of this function and its usage
rg -B 10 -A 5 "if \('interior' in objAsRecord && 'x1'" packages/extension-polkagate/src/util/utils.ts
# Search for any tests or usage of this function
ast-grep --pattern 'function $_(objAsRecord: $_) {
if ("interior" in objAsRecord && "x1" in objAsRecord["interior"]) {
$$$
}
$$$
}'
# Look for any MultiLocation or similar type definitions that might use interior.x1
rg -A 5 "type.*MultiLocation|interface.*MultiLocation"
Length of output: 742
Script:
#!/bin/bash
# Let's find the function name and its usages
rg -B 15 -A 5 "if \('interior' in objAsRecord && 'x1'" packages/extension-polkagate/src/util/utils.ts
# Look for any imports or exports of this function
rg "import.*from.*utils|export.*from.*utils"
# Search for MultiLocation usage in the codebase
rg -A 2 "MultiLocation"
Length of output: 53615
AMIRKHANEF
force-pushed
the
updatePackages
branch
from
618ba61 to
cbe564b
Compare
…ate-extension into updatePackages
Summary by CodeRabbit