fix: devnet #269
fix: devnet #269
Conversation
|
Warning Rate limit exceeded@fmorency has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request introduces a dynamic manifest loading module in Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant MC as ManifestChain Module
participant Env as Environment Config
participant Registry as Chain-Registry
App->>MC: Request manifest data
MC->>Env: Check env.chainTier value
alt chainTier is 'qa'
MC->>Registry: Import development manifest files (assets, chain)
Note over MC: Set manifestIbc to empty array
else chainTier is not 'qa'
MC->>Registry: Import testnet manifest files (assets, chain, ibc)
end
MC-->>App: Return manifestAssets, manifestChain, manifestIbc
Possibly related PRs
Suggested reviewers
Poem
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #269 +/- ##
==========================================
- Coverage 54.47% 54.47% -0.01%
==========================================
Files 206 207 +1
Lines 17712 17736 +24
==========================================
+ Hits 9649 9661 +12
- Misses 8063 8075 +12 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
utils/staking.ts (1)
184-195: Add input validation and documentation.The function logic is correct, but could benefit from:
- Input validation for null/undefined wordArray
- JSDoc documentation explaining the purpose and parameters
Apply this diff to improve the function:
+/** + * Converts a CryptoJS WordArray to Uint8Array + * @param wordArray - The CryptoJS WordArray to convert + * @throws {Error} If wordArray is null or undefined + * @returns {Uint8Array} The converted array + */ function wordArrayToUint8Array(wordArray: CryptoJS.lib.WordArray) { + if (!wordArray) { + throw new Error('wordArray is required'); + } const words = wordArray.words; const sigBytes = wordArray.sigBytes; const u8 = new Uint8Array(sigBytes);config/manifestChain.ts (1)
7-7: Consider tracking mainnet manifest requirement.The TODO comment about mainnet manifest should be tracked for future implementation.
Would you like me to create an issue to track the mainnet manifest requirement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
config/manifestChain.ts(1 hunks)hooks/useFeeEstimation.ts(1 hunks)package.json(2 hunks)pages/_app.tsx(1 hunks)tests/render.tsx(1 hunks)utils/ibc.ts(1 hunks)utils/staking.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
hooks/useFeeEstimation.ts
[error] 29-29: Unexpected constant condition.
(lint/correctness/noConstantCondition)
🪛 GitHub Check: codecov/patch
hooks/useFeeEstimation.ts
[warning] 28-30: hooks/useFeeEstimation.ts#L28-L30
Added lines #L28 - L30 were not covered by tests
config/manifestChain.ts
[warning] 9-9: config/manifestChain.ts#L9
Added line #L9 was not covered by tests
[warning] 17-20: config/manifestChain.ts#L17-L20
Added lines #L17 - L20 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (7)
utils/staking.ts (1)
2-2: LGTM!The change to a named import is more specific and follows modern JavaScript/TypeScript best practices.
package.json (1)
36-36: Verify the dependency versions.The dependency updates look good, but let's verify the latest versions and any potential security advisories.
Also applies to: 58-59
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check latest versions and security advisories for the updated dependencies # Check NPM for latest versions echo "Checking latest versions..." npm view @chain-registry/assets version npm view chain-registry version npm view bech32 version # Check for known vulnerabilities echo "Checking for vulnerabilities..." npm audit @chain-registry/assets@^1.70.122 npm audit chain-registry@^1.69.122 npm audit bech32@^2.0.0Length of output: 1424
Dependency Versions Verified but Vulnerability Scan Inconclusive
- The npm view commands confirm that:
- @chain-registry/assets is at 1.70.122
- chain-registry is at 1.69.122
- bech32 is at 2.0.0
- The vulnerability audit commands failed due to the absence of a package lockfile (use
npm i --package-lock-onlyto generate it). This prevents a proper security advisory check.config/manifestChain.ts (1)
16-26: Add test coverage for manifest initialization.The manifest initialization logic lacks test coverage, particularly for the 'qa' build type path.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check test coverage for manifest initialization # Search for test files related to manifestChain fd -e test.ts -e spec.ts manifestChain # Search for any tests that might be testing this functionality rg -l "manifestChain" $(fd -e test.ts -e spec.ts)Length of output: 120
Missing Test Coverage for Manifest Initialization
The search results indicate that there aren’t any test files directly covering the manifest initialization logic—especially for the 'qa' build type path—in the repository. Only an unrelated reference in
utils/profanityFilter.test.tswas found.
- config/manifestChain.ts: No tests appear to cover the conditional logic that initializes
manifestAssets,manifestChain, andmanifestIbcfor the 'qa' build type.- Test files found: The only hit was in
utils/profanityFilter.test.ts, which is not related to manifest initialization.This confirms the original review comment: additional test coverage is required to validate the manifest initialization logic.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 17-20: config/manifestChain.ts#L17-L20
Added lines #L17 - L20 were not covered by teststests/render.tsx (1)
13-13: LGTM!The import statement has been correctly updated to use the new dynamic manifest loading module.
utils/ibc.ts (1)
8-8: LGTM!The imports and usage of manifest assets and IBC data have been correctly updated to use the new dynamic manifest loading module.
Also applies to: 10-10, 70-70
pages/_app.tsx (2)
45-45: LGTM! Import statement updated to use dynamic manifest loading.The change aligns with the PR's objective to fix devnet by using the new dynamic manifest loading module instead of hardcoded testnet imports.
203-204: Verify chain configuration compatibility.The manifest chain is combined with osmosis and axelar testnet chains. Please ensure that the dynamic manifest configuration is compatible with these testnet chains, especially for cross-chain operations.
Run the following script to check for any potential chain configuration mismatches:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check chain configuration compatibility between manifest and other chains # Search for chain configuration usage patterns rg -A 5 'chain-registry/testnet/(osmosistestnet|axelartestnet)' # Search for IBC configuration ast-grep --pattern 'ibc: { $$$ }'Length of output: 1591
I'll perform a further scan for
"manifestIbc"in the repository to verify if the manifest chain's IBC configuration is defined and aligned with the corresponding testnet IBC configurations.
🏁 Script executed:
#!/bin/bash # Search for manifest IBC configuration in the repository rg 'manifestIbc' -n .Length of output: 539
Chain Configuration Compatibility Check
I reviewed the manifest chain’s configuration in
config/manifestChain.tsand its usage in cross-chain IBC setups (e.g., inutils/ibc.ts). The manifest chain assigns an empty IBC configuration by default (with a comment noting that the devnet doesn’t include IBC) but later assignsibcdynamically. This means that—as long as the dynamic assignment properly provides IBC details—the manifest chain configuration will be compatible with the osmosis and axelar testnet chains used for cross-chain operations.
- Verified: Manifest chain’s IBC configuration is merged with testnet configurations (as seen in
utils/ibc.ts), and the dynamic assignment inconfig/manifestChain.tsensures that a proper IBC configuration is used when available.No mismatches were found in the configuration patterns based on our repository search.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Félix C. Morency <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Félix C. Morency <[email protected]>
|
Superseded by #270 |
This PR bumps the chain registry and provides a fix for the devnet build.
Summary by CodeRabbit
New Features
Chores
Refactor