fix: devnet #270
fix: devnet #270
Conversation
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]>
WalkthroughThis pull request introduces a new configuration module that dynamically builds a manifest based on an environment variable, conditionally loading different manifest files depending on the build type. It updates several parts of the application by replacing hardcoded imports with references to the new manifest module. Additionally, it refines fee estimation logic to standardize the gas price token suffix, bumps dependency versions, adds a new dependency, and introduces a helper function in staking utilities for converting CryptoJS word arrays. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant M as ManifestModule
participant Env as Environment
App->>M: Import manifest module
M->>Env: Read buildType value
alt buildType is undefined
M-->>App: Throw error
else buildType is "qa"
M->>M: Load QA manifest (empty IBC data)
else
M->>M: Conditionally load dev/testnet manifest
end
M-->>App: Export manifestAssets, manifestChain, manifestIbc
sequenceDiagram
participant FeeHook as useFeeEstimation
participant Chain as ChainName
FeeHook->>Chain: Check chainName value
FeeHook-->>FeeHook: Set gas token suffix "umfx"
Possibly related PRs
Suggested labels
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 (6)
utils/staking.ts (1)
184-195: Add input validation to the wordArrayToUint8Array function.While the implementation is correct, consider adding input validation to handle edge cases.
function wordArrayToUint8Array(wordArray: CryptoJS.lib.WordArray) { + if (!wordArray || !wordArray.words || !wordArray.sigBytes) { + throw new Error('Invalid WordArray input'); + } + const words = wordArray.words; const sigBytes = wordArray.sigBytes; const u8 = new Uint8Array(sigBytes);config/manifestChain.ts (2)
4-14: Improve error handling for environment variable.The error message could be more descriptive to help with troubleshooting.
- throw new Error('Chain tier environment variable is not set'); + throw new Error('Required environment variable "chainTier" is not set. Expected values: "qa" or "testnet".');🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 13-13: config/manifestChain.ts#L13
Added line #L13 was not covered by tests
16-20: Consider using dynamic imports for better code splitting.Using
requirefor conditional imports can be replaced with dynamic imports for better tree-shaking and code splitting.-if (buildType === 'qa') { - manifestModule = require('chain-registry/devnet/manifestdevnet'); -} else { - manifestModule = require('chain-registry/testnet/manifesttestnet'); -} +manifestModule = await import( + buildType === 'qa' + ? 'chain-registry/devnet/manifestdevnet' + : 'chain-registry/testnet/manifesttestnet' +);🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 17-17: config/manifestChain.ts#L17
Added line #L17 was not covered by testshooks/useFeeEstimation.ts (1)
28-34: Add test coverage for fee estimation with devnet.The changes to gas price token suffix logic lack test coverage, particularly for the 'manifestdevnet' case.
Additionally, consider extracting the token suffix logic to a configuration file for better maintainability:
+const GAS_PRICE_TOKENS = { + manifesttestnet: 'umfx', + manifestdevnet: 'umfx', + default: 'uosmo' +}; + GasPrice.fromString( - `${gasPrice}${ - chainName === 'manifesttestnet' || chainName === 'manifestdevnet' - ? 'umfx' - : 'uosmo' - }` + `${gasPrice}${GAS_PRICE_TOKENS[chainName] || GAS_PRICE_TOKENS.default}` )❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check test coverage for fee estimation # Find test files related to fee estimation fd -e test.ts -e spec.ts -e test.tsx -e spec.tsx . | grep -i fee # Check if there are any tests for devnet fee estimation rg -l "manifestdevnet.*useFeeEstimation" -g '*.test.{ts,tsx}' -g '*.spec.{ts,tsx}'Length of output: 151
Action: Add test coverage for the manifestdevnet case in fee estimation.
Our investigation did not uncover any test files or references covering the devnet scenario in
useFeeEstimation.ts, confirming that this new behavior lacks test coverage. Please add tests to verify that whenchainNameis set to 'manifestdevnet', the fee estimation logic correctly appends the 'umfx' suffix. Additionally, consider refactoring the token suffix logic into a configuration object for improved maintainability.🧰 Tools
🪛 ESLint
[error] 30-32: Replace
⏎············?·'umfx'⏎···········with·?·'umfx'(prettier/prettier)
🪛 GitHub Check: codecov/patch
[warning] 28-34: hooks/useFeeEstimation.ts#L28-L34
Added lines #L28 - L34 were not covered by testspages/_app.tsx (2)
183-195: Consider extracting endpoint configuration.The endpoint configuration is currently embedded in the component. Consider extracting it to the manifest configuration module for better maintainability and consistency with the new dynamic configuration approach.
- const endpointOptions = { - isLazy: true, - endpoints: { - [env.chain]: { - rpc: [env.rpcUrl], - rest: [env.apiUrl], - }, - ['osmosistestnet']: { - rpc: [env.osmosisRpcUrl], - rest: [env.osmosisApiUrl], - }, - }, - }; + const endpointOptions = getEndpointOptions(env);Create a new function in the manifest configuration module:
// @/config/manifestChain.ts export function getEndpointOptions(env: typeof import('@/config/env').default) { return { isLazy: true, endpoints: { [env.chain]: { rpc: [env.rpcUrl], rest: [env.apiUrl], }, ['osmosistestnet']: { rpc: [env.osmosisRpcUrl], rest: [env.osmosisApiUrl], }, }, }; }
215-219: Update metadata in WalletConnect configuration.The WalletConnect metadata appears to be using placeholder values. Update these with appropriate production values:
- name: "Alberto"
- description: "Manifest Network Web App"
- url: "https://alberto.com"
metadata: { - name: 'Alberto', - description: 'Manifest Network Web App', - url: 'https://alberto.com', + name: 'Manifest Network', + description: 'Manifest Network - Web3 Application', + url: 'https://manifest.network', icons: [], },
📜 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
🪛 ESLint
hooks/useFeeEstimation.ts
[error] 30-32: Replace ⏎············?·'umfx'⏎··········· with ·?·'umfx'
(prettier/prettier)
🪛 GitHub Check: codecov/patch
hooks/useFeeEstimation.ts
[warning] 28-34: hooks/useFeeEstimation.ts#L28-L34
Added lines #L28 - L34 were not covered by tests
config/manifestChain.ts
[warning] 13-13: config/manifestChain.ts#L13
Added line #L13 was not covered by tests
[warning] 17-17: config/manifestChain.ts#L17
Added line #L17 was not covered by tests
[warning] 25-28: config/manifestChain.ts#L25-L28
Added lines #L25 - L28 were not covered by tests
🪛 GitHub Actions: Prettier
hooks/useFeeEstimation.ts
[warning] 1-1: Code style issues found in the above file. Run Prettier with --write to fix.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (6)
utils/staking.ts (1)
2-2: LGTM! Import style improvement.The change from namespace import to named import for bech32 follows better practices by being more explicit about what's being imported.
package.json (1)
36-36: Verify compatibility of updated dependencies.Let's check for any breaking changes in the updated packages.
Also applies to: 58-59
❌ Verification inconclusive
🌐 Web query:
What are the latest versions and any breaking changes for: 1. @chain-registry/assets 2. bech32 3. chain-registryError during web search
Review: Dependency Version Compatibility
The patch-level updates for @chain-registry/assets (^1.70.122) and chain-registry (^1.69.122) are generally expected to be backward compatible under semantic versioning. However, the addition of bech32 at a new major version (^2.0.0) means there could be breaking changes compared to the previous version. Since our initial web query for breaking changes did not return results, please verify—by checking the official release notes or documentation for bech32 v2.0.0—whether its API changes impact our usage in the codebase.
tests/render.tsx (1)
13-13: LGTM!The import statement has been correctly updated to use the new manifest configuration module.
utils/ibc.ts (1)
8-8: LGTM!The import statement and IBC data initialization have been correctly updated to use the new manifest configuration module.
Also applies to: 70-70
pages/_app.tsx (2)
44-45: LGTM! Import changes align with dynamic manifest configuration.The change to import from
@/config/manifestChaininstead of directly from chain-registry supports the PR's objective of fixing devnet configuration by allowing dynamic manifest loading based on environment.
203-204: Verify chain configuration compatibility.The ChainProvider is configured with multiple chains (manifest, osmosis, axelar). Please ensure that the new manifest configuration from
@/config/manifestChainmaintains compatibility with the other chain configurations, particularly for cross-chain operations.Run this script to verify chain configuration compatibility:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for potential chain configuration mismatches # Check chain configurations ast-grep --pattern 'chain as manifestChain' ast-grep --pattern 'assets as manifestAssets' # Check usage in cross-chain operations rg -A 5 'manifestChain|manifestAssets'Length of output: 2617
Chain configuration compatibility is maintained.
ThemanifestChainandmanifestAssetsfromconfig/manifestChain.tsare structured and exported similarly to the other chain configurations. They’re used consistently in the ChainProvider (inpages/_app.tsx), in tests, and in cross-chain utilities (e.g.,utils/ibc.ts). Notably, while themanifestIbcfield conditionally returns an empty array in QA builds—which is an intentional design decision—the overall configuration remains compatible with cross-chain operations.
| if (buildType === 'qa') { | ||
| const { assets, chain } = manifestModule; | ||
| manifestAssets = assets; | ||
| manifestChain = chain; | ||
| manifestIbc = []; // Default value since devnet doesn't include ibc | ||
| } else { | ||
| const { assets, chain, ibc } = manifestModule; | ||
| manifestAssets = assets; | ||
| manifestChain = chain; | ||
| manifestIbc = ibc!; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add test coverage for manifest initialization.
The manifest initialization logic lacks test coverage, which is crucial for ensuring correct behavior across different build types.
🏁 Script executed:
#!/bin/bash
# Description: Check test coverage for manifest initialization
# Find test files related to manifest configuration
fd -e test.ts -e spec.ts -e test.tsx -e spec.tsx . | grep -i manifest
# Check if there are any tests importing manifestChain
rg -l "from ['\"]\@\/config\/manifestChain['\"]" -g '*.test.{ts,tsx}' -g '*.spec.{ts,tsx}'Length of output: 282
I'm going to re-run the test search with an updated command that avoids regex escape issues. We'll use rg -F for fixed-string matching. Please run the following script:
Once we have the output, we can determine whether tests exist for the manifest initialization logic or if additional coverage needs to be added.
🏁 Script executed:
#!/bin/bash
echo "Searching for tests that import manifestChain..."
rg -l -F "@/config/manifestChain" -g "*.test.ts" -g "*.spec.ts" -g "*.test.tsx" -g "*.spec.tsx" .Length of output: 204
Manifest Initialization Test Coverage Missing
Our investigation confirms that no tests are importing or referencing @/config/manifestChain. This indicates that the manifest initialization logic (i.e., setting of manifestAssets, manifestChain, and manifestIbc for different build types) currently lacks test coverage.
- File:
config/manifestChain.ts - Issue: No tests found covering manifest initialization logic
Please add tests that simulate different buildType values (e.g., 'qa' and others) to validate that assets, chain, and ibc are correctly initialized based on the manifest module.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 25-28: config/manifestChain.ts#L25-L28
Added lines #L25 - L28 were not covered by tests
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #270 +/- ##
==========================================
+ Coverage 54.47% 54.48% +0.01%
==========================================
Files 206 207 +1
Lines 17712 17727 +15
==========================================
+ Hits 9649 9659 +10
- Misses 8063 8068 +5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
hooks/useFeeEstimation.ts (2)
5-35: Add test coverage for fee estimation with the standardized token.The fee estimation logic with the standardized 'umfx' token is not covered by tests. Given the critical nature of fee estimation in blockchain transactions, please add test cases to verify:
- Fee calculation with the standard gas price token
- Error handling for invalid client states
- Gas estimation with different modifiers
Would you like me to help generate comprehensive test cases for the fee estimation logic?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 28-28: hooks/useFeeEstimation.ts#L28
Added line #L28 was not covered by tests
5-6: Consider documenting or refactoring the chainName parameter.The
chainNameparameter is still required but no longer influences the gas price token suffix. Consider either:
- Documenting why the parameter is needed despite not affecting the token suffix
- Refactoring to make the parameter usage more explicit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hooks/useFeeEstimation.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
hooks/useFeeEstimation.ts
[warning] 28-28: hooks/useFeeEstimation.ts#L28
Added line #L28 was not covered by tests
🔇 Additional comments (1)
hooks/useFeeEstimation.ts (1)
28-28: Verify the implications of standardizing the gas price token.The gas price token suffix has been standardized to 'umfx', removing the previous chain-specific logic. While this aligns with the devnet fix objective, please verify that this won't affect other chains or environments.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other uses of gas price tokens in the codebase echo "Searching for gas price token references..." rg -i "umfx|uosmo" --type tsLength of output: 3861
Gas Price Token Standardization Verified
- The search indicates that the
umfxdenomination is used consistently across the codebase (e.g., in fee queries, UI components, and tests), which confirms that the change inhooks/useFeeEstimation.tsaligns with our current devnet configuration.- However, note that the hook still accepts a
chainNameparameter even though it no longer affects the token suffix. If the intention is to support only devnet (withumfx), consider either removing or clearly documenting this parameter to prevent confusion.- As the modified line is currently not covered by tests—and given the critical nature of fee estimation—it is recommended to add tests to validate that fee estimation works as expected.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 28-28: hooks/useFeeEstimation.ts#L28
Added line #L28 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
config/manifestChain.ts (3)
11-11: Track mainnet manifest implementation.The TODO comment indicates pending work for mainnet support.
Would you like me to create an issue to track the implementation of mainnet manifest support?
13-17: Consider enhancing manifest loading logic.A few suggestions to improve the manifest loading:
- Consider using dynamic imports instead of require() for better tree-shaking and code splitting.
- Add error handling for invalid build types.
Here's a suggested implementation:
-if (buildType === 'qa') { - manifestModule = require('chain-registry/devnet/manifestdevnet'); -} else { - manifestModule = require('chain-registry/testnet/manifesttestnet'); -} +const getManifestModule = async () => { + switch (buildType) { + case 'qa': + return import('chain-registry/devnet/manifestdevnet'); + case 'testnet': + return import('chain-registry/testnet/manifesttestnet'); + default: + throw new Error(`Unsupported build type: ${buildType}`); + } +}; +manifestModule = await getManifestModule();🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 14-14: config/manifestChain.ts#L14
Added line #L14 was not covered by tests
21-31: Refactor manifest initialization to reduce duplication.The current implementation has duplicated initialization logic and uses a potentially unsafe non-null assertion.
Here's a suggested implementation that reduces duplication and improves type safety:
-if (buildType === 'qa') { - const { assets, chain } = manifestModule; - manifestAssets = assets; - manifestChain = chain; - manifestIbc = []; // Default value since devnet doesn't include ibc -} else { - const { assets, chain, ibc } = manifestModule; - manifestAssets = assets; - manifestChain = chain; - manifestIbc = ibc!; -} +const { assets, chain, ibc } = manifestModule; +manifestAssets = assets; +manifestChain = chain; +manifestIbc = buildType === 'qa' ? [] : (ibc ?? []);🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 22-25: config/manifestChain.ts#L22-L25
Added lines #L22 - L25 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/manifestChain.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
config/manifestChain.ts
[warning] 14-14: config/manifestChain.ts#L14
Added line #L14 was not covered by tests
[warning] 22-25: config/manifestChain.ts#L22-L25
Added lines #L22 - L25 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
config/manifestChain.ts (3)
1-9: LGTM! Clean imports and type definitions.The imports and type definitions are well-structured and provide good type safety for the manifest data.
21-31: Add test coverage for manifest initialization.The manifest initialization logic lacks test coverage, which is crucial for ensuring correct behavior across different build types.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 22-25: config/manifestChain.ts#L22-L25
Added lines #L22 - L25 were not covered by tests
33-33: LGTM! Clean exports.The exports are well-defined and provide access to all necessary manifest data.
See #269
Summary by CodeRabbit
New Features
CryptoJS.lib.WordArrayintoUint8Arrayfor improved data handling.Chores
Refactor
Tests