Fix Bug with Wallet Address, Update Wasm Command, and Update Denom Utils #120
Conversation
WalkthroughThis pull request introduces several changes across multiple files, including the addition of a new changeset entry in Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (19)
.changeset/wild-oranges-give.md (1)
Line range hint
1-8: Consider using minor/major version bumps instead of patch.The changes described in the PR objectives include new features (Wasm command enhancements) and potentially breaking changes (denom utils modifications), which typically warrant minor or major version bumps according to semver:
- Minor version for new features (Wasm command changes)
- Major version for breaking changes (if denom utils changes affect existing behavior)
Additionally, the changeset description could be more detailed to help users understand the impact of these changes.
Consider this improved structure:
--- -"@andromedaprotocol/andromeda.js": patch -"@andromedaprotocol/cli": patch -"@andromedaprotocol/gql": patch +"@andromedaprotocol/andromeda.js": minor +"@andromedaprotocol/cli": minor +"@andromedaprotocol/gql": minor --- NPM package updates and Wallet Address cache in CLI + +Changes: +- Fixed: Wallet address now persists correctly when changing wallet/chain +- Added: Enhanced Wasm command with raw query support using keys +- Fixed: Improved denom utils to handle macro/micro conversions accurately +- Added: New test coverage for denom utilitiespackages/andrjs/tsconfig.build.json (1)
14-15: LGTM! Consider consolidating test exclusion patterns.The exclusion patterns correctly prevent test files from being included in the production build. However, for better maintainability, you could consolidate these patterns.
Consider this alternative pattern that covers both cases:
"exclude": [ "node_modules", "dist", - "test/**/*.ts", - "**/*.spec.ts" + "**/*.(spec|test).ts" ]This pattern:
- Excludes both .spec.ts and .test.ts files
- Works for files in any directory
- Reduces maintenance overhead
- Is more consistent with common TypeScript project structures
packages/cli/tsconfig.build.json (1)
7-16: LGTM! Consider standardizing test file patterns.The configuration correctly excludes test files from the build output. However, there's an opportunity to standardize the test file patterns.
Currently, you have two patterns for test files:
test/**/*.tssrc/**/*spec.tsConsider using a consistent naming convention across the project. For example, either:
- Use
.spec.tsfor all test files and move them next to the source files- Keep all test files in the
testdirectoryThis would allow you to simplify the exclude patterns to just one pattern.
.github/workflows/test.yml (1)
1-22: Consider adding test result artifacts and coverage reports.To improve debugging and maintain quality metrics, consider adding test result artifacts and coverage reporting.
Add these steps after the test step:
- name: Upload test results if: always() uses: actions/upload-artifact@v3 with: name: test-results path: | packages/**/junit.xml packages/**/coverage/ retention-days: 30 - name: Upload coverage to Codecov uses: codecov/codecov-action@v3 with: files: ./packages/**/coverage/coverage-final.jsonpackages/andrjs/CHANGELOG.md (1)
7-10: Consider adding more detailed changelog entries.The current entries could be more descriptive to help users understand the impact and context of changes. Consider:
- Adding specific details about the denom utilities fix
- Including examples of the decimal places fix
- Mentioning the test coverage details
- Adding references to related issues/PRs
Example format:
-Fix: Denom utilities now return the correct amount of decimal places +Fix: Denom utilities now preserve decimal places during macro/micro conversions (#120) -Tests: Added tests for denom utilities +Tests: Added comprehensive test suite for denom utilities covering decimal precision edge casespackages/cli/CHANGELOG.md (1)
7-11: Enhance changelog entries with more details.The changelog entries could be more descriptive to help users understand the impact of changes:
- "NPM package updates" - Which packages and why?
- "Wallet Address cache" - What's the benefit?
- "GQL Assets default to app-contract" - What's the impact?
- Command renames - Consider adding examples of the new usage
Consider updating the entries like this:
-NPM package updates -Wallet Address cache -GQL Assets default to app-contract -wasm query-raw command changed to query raw key +- Updated NPM dependencies for improved stability and security +- Added Wallet Address caching to persist addresses across chain/wallet changes +- Changed GQL Assets to default to app-contract for consistent behavior +- Renamed commands for better clarity: + - `wasm query-raw` → `query raw key` + - Example usage: `andromeda query raw key <contract> <key>`packages/andrjs/src/utils/denom.spec.ts (5)
4-13: Consider adding more edge cases to micro-to-macro conversion tests.While the current test cases cover basic scenarios well, consider adding tests for:
- Negative numbers
- Invalid inputs (non-numeric strings)
- Invalid denomination types
Example additions:
expect(() => convertMicroToMacro('-1000000', 'u')).toThrow(); expect(() => convertMicroToMacro('abc', 'u')).toThrow(); expect(() => convertMicroToMicro('1000000', 'x')).toThrow();
15-24: Consider adding more edge cases to macro-to-micro conversion tests.Similar to the micro-to-macro tests, consider adding tests for:
- Negative numbers
- Invalid inputs (non-numeric strings)
- Invalid denomination types
Example additions:
expect(() => convertMacroToMicro('-1.0', 'u')).toThrow(); expect(() => convertMacroToMicro('abc', 'u')).toThrow(); expect(() => convertMacroToMicro('1.0', 'x')).toThrow();
26-30: Enhance international formatting test coverage.Consider adding test cases for:
- Negative numbers
- Very large numbers (billions/trillions)
- Different locales
- Edge cases with zero decimal places
Example additions:
expect(formatAmountToInternational(-1234.56, 2)).toBe('-1,234.56'); expect(formatAmountToInternational(1234567890.12, 2)).toBe('1,234,567,890.12'); expect(formatAmountToInternational(0.00001, 5)).toBe('0.00001');
32-45: Strengthen DENOM_EXPONENTS validation.Consider adding tests to verify:
- Individual exponent values (e.g., 'u' === 6)
- Immutability of the constant
Example additions:
test('DENOM_EXPONENTS values are correct and immutable', () => { expect(DENOM_EXPONENTS.u).toBe(6); expect(() => { DENOM_EXPONENTS.u = 7; }).toThrow(); });
47-50: Expand units config retrieval test coverage.The current tests only cover 'inj' and 'uatom'. Consider adding test cases for:
- Invalid denominations
- Other common tokens (e.g., 'usdc', 'eth')
- Case sensitivity checks
Example additions:
test('getUnitsConfigFromDenom handles various cases', () => { expect(() => getUnitsConfigFromDenom('')).toThrow(); expect(() => getUnitsConfigFromDenom('invalid')).toThrow(); expect(getUnitsConfigFromDenom('UATOM')).toEqual( expect.objectContaining({ macroDenom: 'ATOM' }) ); expect(getUnitsConfigFromDenom('usdc')).toEqual( expect.objectContaining({ units: 6, macroDenom: 'USDC' }) ); });packages/cli/src/handlers/gql/index.ts (1)
Line range hint
99-103: Enhance type safety and input validation.While the default values work, there are several improvements that could be made:
- Use TypeScript type annotations for better type safety
- Add input validation for limit/offset
- Convert numeric defaults at the type level rather than runtime
Consider refactoring to:
- const { type = "app-contract", search } = flags; - const limit = parseInt(flags.limit ?? '10'); - const offset = parseInt(flags.offset ?? '0'); + interface AssetFlags extends Flags { + type?: string; + search?: string; + limit?: string; + offset?: string; + } + + const { type = "app-contract", search } = flags as AssetFlags; + const limit = Math.max(0, parseInt(flags.limit ?? '10')); + const offset = Math.max(0, parseInt(flags.offset ?? '0')); + + if (isNaN(limit) || isNaN(offset)) { + throw new Error("Invalid limit or offset value"); + }packages/andrjs/src/utils/denom.ts (1)
Line range hint
1-144: Consider architectural improvements for better maintainability.To enhance the codebase, consider these architectural improvements:
- Add a custom type for amounts to ensure type safety:
type Amount = { value: string; denom: string; };
Consider using a decimal arithmetic library like
decimal.jsorbignumber.jsto handle precision-critical calculations more safely.Add input validation as a separate utility function to maintain DRY principle.
Consider adding logging for debugging and monitoring purposes.
Would you like me to provide a detailed implementation for any of these suggestions?
packages/andrjs/src/AndromedaClient.ts (2)
207-214: Add error handling for JSON parsing and update documentation.While the implementation looks good, consider these improvements:
- Add try-catch for JSON parsing to handle malformed responses gracefully
- Update the JSDoc to reflect the new
keyparameter typeApply this diff:
/** * Wrapper function for CosmWasm query * https://cosmos.github.io/cosmjs/latest/cosmwasm-stargate/classes/SigningCosmWasmClient.html#queryContractSmart * @param address + * @param key The key to query, either as UTF-8 string or Uint8Array * @returns */ async queryContractRaw<T = any>(address: string, key: string | Uint8Array) { this.preMessage(); const result = await this.chainClient!.queryClient!.queryContractRaw( address, typeof key === 'string' ? Buffer.from(key, 'utf8') : key ); if (!result || result.length === 0) return null; - return JSON.parse(Buffer.from(result).toString('utf8')) as T + try { + return JSON.parse(Buffer.from(result).toString('utf8')) as T; + } catch (error) { + throw new Error(`Failed to parse contract response: ${error.message}`); + } }
Line range hint
224-238: Enhance type safety and documentation.The implementation looks good, but could benefit from these improvements:
- Add an interface for the return type
- Enhance the documentation with parameter and return type descriptions
Apply this diff:
+ interface ContractStateResponse { + states: Array<{ + key: string; + value: string; + }>; + pagination: { + nextKey: Uint8Array; + total: number; + }; + } + /** * Wrapper function for CosmWasm query * https://cosmos.github.io/cosmjs/latest/cosmwasm-stargate/classes/SigningCosmWasmClient.html#queryContractSmart - * @param address - * @param query - * @returns + * @param address The contract address to query + * @param pagination Pagination options for the query + * @returns {Promise<ContractStateResponse>} Object containing states array and pagination info */ - async queryContractStates(address: string, pagination: Partial<PageRequest>) { + async queryContractStates( + address: string, + pagination: Partial<PageRequest> + ): Promise<ContractStateResponse> {packages/cli/src/state/WalletStore.ts (1)
260-261: Add error handling for empty accounts array.The implementation correctly caches the wallet address, fixing the reported issue. However, consider adding error handling for cases where
getAccounts()might return an empty array.Consider this safer implementation:
- const address = await signer.getAccounts().then(accounts => accounts[0].address); + const accounts = await signer.getAccounts(); + if (!accounts.length) { + throw new Error('No accounts found for the wallet'); + } + const address = accounts[0].address; this.updateWallet(wallet.name, { addresses: { [wallet.prefix]: address } })packages/cli/src/handlers/wasm.ts (3)
82-82: Correct grammatical error in descriptionThe description should be grammatically correct for better clarity. It should read "Queries contract states" without the extra 'a'.
Apply this diff to fix the description:
description: "Queries a contract states", - description: "Queries a contract states", + description: "Queries contract states",
293-293: Fix typo in log messageThere's a typo in the log message "Querying contrac key...". It should be "Querying contract key..." to accurately reflect the action.
Apply this diff to correct the typo:
"Querying contrac key...", - "Querying contrac key...", + "Querying contract key...",
Line range hint
331-338: Check for undefined 'nextKey' before using 'Buffer.from'If
states.pagination.nextKeyis undefined, usingBuffer.fromon it will throw an error. It's important to check ifnextKeyexists before attempting to convert it.Apply this diff to add the necessary checks:
if (states.pagination) { + if (states.pagination.nextKey) { console.log(pc.gray(`Next Key - ${Buffer.from(states.pagination.nextKey).toString('utf8')}`)); console.log(pc.gray(`Next Key Bytes - ${Buffer.from(states.pagination.nextKey).toString('hex')}`)); + } else { + console.log(pc.gray(`Next Key - None`)); + console.log(pc.gray(`Next Key Bytes - None`)); + } console.log(pc.gray(`Total - ${states.pagination.total.toString()}`)); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
packages/gql/CHANGELOG.mdis excluded by!packages/gql/**packages/gql/package.jsonis excluded by!packages/gql/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
- .changeset/pre.json (1 hunks)
- .changeset/wild-oranges-give.md (1 hunks)
- .github/workflows/test.yml (1 hunks)
- package.json (2 hunks)
- packages/andrjs/CHANGELOG.md (1 hunks)
- packages/andrjs/jest.config.js (1 hunks)
- packages/andrjs/package.json (3 hunks)
- packages/andrjs/src/AndromedaClient.ts (2 hunks)
- packages/andrjs/src/utils/denom.spec.ts (1 hunks)
- packages/andrjs/src/utils/denom.ts (2 hunks)
- packages/andrjs/tsconfig.build.json (1 hunks)
- packages/cli/CHANGELOG.md (1 hunks)
- packages/cli/package.json (1 hunks)
- packages/cli/src/cmd.ts (1 hunks)
- packages/cli/src/handlers/gql/index.ts (2 hunks)
- packages/cli/src/handlers/wasm.ts (3 hunks)
- packages/cli/src/index.spec.ts (1 hunks)
- packages/cli/src/schema/SchemaPrompt.test.ts (0 hunks)
- packages/cli/src/state/WalletStore.ts (1 hunks)
- packages/cli/tsconfig.build.json (1 hunks)
💤 Files with no reviewable changes (1)
- packages/cli/src/schema/SchemaPrompt.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/andrjs/jest.config.js
- packages/cli/package.json
- packages/cli/src/cmd.ts
🔇 Additional comments (14)
.changeset/pre.json (1)
13-14: LGTM! The changeset entry has been properly added.The addition of "wild-oranges-give" to the changesets array maintains the correct JSON structure and follows the existing naming convention. This changeset will appropriately track the package updates related to the wallet address fix, Wasm command updates, and denom utils improvements.
packages/cli/src/index.spec.ts (2)
1-2: LGTM! Clean and focused imports.The imports are correctly structured, importing only what's needed for the test.
1-11: Verify test file location and configuration.The test file is in the source directory (
src/). Consider moving it to a dedicated__tests__directory for better organization.Let's check the test file organization in the project:
package.json (2)
34-35: LGTM: Security-related dependency override.The addition of the dset override follows the same pattern as other security-related version constraints in the file, ensuring consistent security across packages.
8-8: ```shell
#!/bin/bashLet's try a different approach to find test files and directories
1. Look for test directories
fd -t d "test" packages/
2. Look for files with "test" or "spec" in their names using different patterns
fd -t f "(test|spec).(ts|js|tsx|jsx|mjs)$" packages/
3. Check if there are any Jest or Vitest config files that might indicate test setup
fd -t f "(jest|vitest).config.(ts|js|mjs)$"
4. Look at package.json files in packages to see test scripts
fd -t f "package.json" packages/ -x grep -l ""test":"
</blockquote></details> <details> <summary>packages/andrjs/CHANGELOG.md (1)</summary><blockquote> `10-10`: **Verify if Uint8Array support requires a minor/major version bump.** The addition of Uint8Array key support in Query Contract Raw could potentially be a breaking change or new feature rather than a patch, depending on how it affects existing implementations. Let's check the impact: <details> <summary>✅ Verification successful</summary> Let me gather more specific information about the implementation changes. --- **This change should remain as a patch version bump** The modification to `queryContractRaw` is backward compatible since: 1. It maintains the existing string parameter functionality unchanged (`Buffer.from(key, 'utf8')`) 2. Only adds support for an additional input type (`Uint8Array`) without modifying the return type or behavior 3. The implementation shows this is purely an enhancement that widens the accepted input types without breaking existing string-based usage This follows semantic versioning principles where backward compatible bug fixes should use patch version bumps. </details> <details> <summary>🏁 Scripts executed</summary> The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Description: Check for existing usage of queryContractRaw to determine if this is a breaking change # Search for queryContractRaw usage patterns echo "Searching for queryContractRaw usage patterns..." rg -A 3 "queryContractRaw" # Look for any existing tests that might need updates echo "Checking for related tests..." rg -l "queryContractRaw.*test|spec|test.*queryContractRaw"Length of output: 2784
Script:
#!/bin/bash # Check the git history for the specific change git log -p | rg -A 10 -B 10 "queryContractRaw.*Uint8Array" # Check the implementation before this change git log -p packages/andrjs/src/AndromedaClient.ts | rg -A 10 -B 10 "queryContractRaw"Length of output: 3813
packages/andrjs/package.json (3)
3-3: Version bump aligns with PR scope.The version increment to
2.0.0-beta.3appropriately reflects the significant changes mentioned in the PR objectives, including wallet address fixes, Wasm command updates, and denom utils improvements.
50-50: Test infrastructure additions support PR objectives.The addition of Jest-related dependencies (
@types/jestandts-jest) aligns with the PR's objective to add tests for denom utilities. These are industry-standard testing tools for TypeScript projects.Also applies to: 56-56
71-72: Security override added for dset package.The addition of
"dset@<3.1.4": ">=3.1.4"is a security-focused override that ensures the use of a patched version of the dset package.Let's verify if this security override is necessary by checking for any security advisories:
packages/cli/CHANGELOG.md (1)
12-13: LGTM: Dependency update is properly documented.The dependency update to
@andromedaprotocol/[email protected]is clearly specified.
packages/andrjs/src/utils/denom.spec.ts (1)
1-3: LGTM! Well-structured test organization.The test file follows Jest best practices with clear imports and proper test organization using describe blocks.
packages/cli/src/handlers/gql/index.ts (2)
32-32: LGTM: Improved flag descriptions with default values.The updated flag descriptions now clearly communicate default values to users, improving the CLI's usability.
Also applies to: 40-40, 44-44
98-99: Verify wallet address handling.Given the PR's focus on fixing wallet address storage issues, let's verify the wallet address handling implementation.
✅ Verification successful
Wallet address handling implementation is correct
The implementation properly handles wallet address verification:
WalletStorecorrectly manages wallet address retrieval with proper null checks- The GQL handler appropriately throws an error when no wallet is assigned
- Error message is clear and descriptive: "No wallet currently assigned"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for wallet address handling implementations echo "Searching for wallet address implementations..." rg -l "currentWalletAddress" # Search for wallet state management echo "\nSearching for wallet state management..." rg -l "State\.wallets"Length of output: 920
Script:
#!/bin/bash # Check the implementation of currentWalletAddress and error handling echo "Checking WalletStore implementation..." rg -A 5 "currentWalletAddress" packages/cli/src/state/WalletStore.ts # Check how wallet address is used in the GQL handler echo "\nChecking GQL handler implementation..." rg -B 2 -A 5 "walletAddr" packages/cli/src/handlers/gql/index.ts # Check wallet state initialization echo "\nChecking wallet state initialization..." rg -A 5 "State\.wallets" packages/cli/src/cmd.tsLength of output: 1584
packages/andrjs/src/utils/denom.ts (1)
52-58:⚠️ Potential issueFix syntax error and add input validation.
There are several issues to address:
- Double semicolon on line 53.
- Missing input validation.
- Potential precision loss with large numbers.
Apply these fixes:
export const convertMacroToMicro = (amount: string, unit: UNITS) => { + if (!/^-?\d+\.?\d*$/.test(amount)) { + throw new Error('Invalid amount format'); + } const numZeroes = typeof unit === 'number' ? unit : DENOM_EXPONENTS[unit]; let [result, decimals = ''] = amount.split('.'); + const isNegative = result.startsWith('-'); + result = isNegative ? result.slice(1) : result; const shiftedDecimals = decimals.substring(0, numZeroes).padEnd(numZeroes, '0'); - const remainingDecimals = decimals.substring(numZeroes).replace(/0+$/, '');; + const remainingDecimals = decimals.substring(numZeroes).replace(/0+$/, ''); result = result.concat(shiftedDecimals).replace(/^0+/, '').padEnd(1, '0'); if (remainingDecimals) { result = result.concat('.').concat(remainingDecimals); } - return result; + return `${isNegative ? '-' : ''}${result}`;Let's verify the test coverage for these functions:
| describe("getPackageVersion", () => { | ||
| it("should return the correct version of the current package", () => { | ||
| const packageInfo = getCurrentPackage(); | ||
| expect(packageInfo.version).toBeDefined(); | ||
| expect(typeof packageInfo.version).toBe("string"); | ||
| expect(packageInfo.version).toBe(packageJson.version); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing test coverage with additional test cases.
While the current test case validates the basic functionality, consider adding:
- Error cases (e.g., when package.json is malformed)
- Tests for other properties returned by
getCurrentPackage - Mock package.json to avoid dependency on the actual file
Here's a suggested enhancement:
describe("getPackageVersion", () => {
+ const mockPackageJson = {
+ version: '1.0.0',
+ name: '@andromedaprotocol/cli'
+ };
+
+ beforeEach(() => {
+ jest.mock('../package.json', () => mockPackageJson);
+ });
+
it("should return the correct version of the current package", () => {
const packageInfo = getCurrentPackage();
expect(packageInfo.version).toBeDefined();
expect(typeof packageInfo.version).toBe("string");
- expect(packageInfo.version).toBe(packageJson.version);
+ expect(packageInfo.version).toBe(mockPackageJson.version);
});
+
+ it("should return the correct package name", () => {
+ const packageInfo = getCurrentPackage();
+ expect(packageInfo.name).toBe(mockPackageJson.name);
+ });
+
+ it("should handle missing package.json gracefully", () => {
+ jest.mock('../package.json', () => {
+ throw new Error('File not found');
+ });
+ expect(() => getCurrentPackage()).toThrow();
+ });
});Committable suggestion was skipped due to low confidence.
.github/workflows/test.yml
Outdated
| build: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v3 | ||
| - uses: actions/setup-node@v3 | ||
| with: | ||
| node-version: "20.x" | ||
| registry-url: "https://registry.npmjs.org" | ||
| - run: pnpm install | ||
| - run: pnpm run build | ||
| - run: pnpm run test |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize the job configuration for better performance and reliability.
The current configuration could benefit from caching, timeouts, and proper pnpm setup.
Apply these improvements:
jobs:
build:
runs-on: ubuntu-latest
+ timeout-minutes: 15
steps:
- uses: actions/checkout@v3
+ - uses: pnpm/action-setup@v2
+ with:
+ version: 8
- uses: actions/setup-node@v3
with:
node-version: "20.x"
registry-url: "https://registry.npmjs.org"
+ cache: 'pnpm'
+ - name: Get pnpm store directory
+ shell: bash
+ run: |
+ echo "STORE_PATH=$(pnpm store path --silent)" >> $GITHUB_ENV
+ - uses: actions/cache@v3
+ with:
+ path: ${{ env.STORE_PATH }}
+ key: ${{ runner.os }}-pnpm-store-${{ hashFiles('**/pnpm-lock.yaml') }}
+ restore-keys: |
+ ${{ runner.os }}-pnpm-store-
- run: pnpm install
- run: pnpm run build
- - run: pnpm run test
+ - name: Run tests
+ run: pnpm run test
+ env:
+ CI: true📝 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.
| build: | |
| runs-on: ubuntu-latest | |
| steps: | |
| - uses: actions/checkout@v3 | |
| - uses: actions/setup-node@v3 | |
| with: | |
| node-version: "20.x" | |
| registry-url: "https://registry.npmjs.org" | |
| - run: pnpm install | |
| - run: pnpm run build | |
| - run: pnpm run test | |
| build: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 15 | |
| steps: | |
| - uses: actions/checkout@v3 | |
| - uses: pnpm/action-setup@v2 | |
| with: | |
| version: 8 | |
| - uses: actions/setup-node@v3 | |
| with: | |
| node-version: "20.x" | |
| registry-url: "https://registry.npmjs.org" | |
| cache: 'pnpm' | |
| - name: Get pnpm store directory | |
| shell: bash | |
| run: | | |
| echo "STORE_PATH=$(pnpm store path --silent)" >> $GITHUB_ENV | |
| - uses: actions/cache@v3 | |
| with: | |
| path: ${{ env.STORE_PATH }} | |
| key: ${{ runner.os }}-pnpm-store-${{ hashFiles('**/pnpm-lock.yaml') }} | |
| restore-keys: | | |
| ${{ runner.os }}-pnpm-store- | |
| - run: pnpm install | |
| - run: pnpm run build | |
| - name: Run tests | |
| run: pnpm run test | |
| env: | |
| CI: true |
| import { convertMicroToMacro, convertMacroToMicro, formatAmountToInternational, DENOM_EXPONENTS, getUnitsConfigFromDenom } from './denom'; | ||
|
|
||
| describe('Denom Utility Functions', () => { | ||
| test('converts micro to macro correctly', () => { | ||
| expect(convertMicroToMacro('1000000', 'u')).toBe('1.0'); | ||
| expect(convertMicroToMacro('1000000', 6)).toBe('1.0'); | ||
| expect(convertMicroToMacro('500000', 'm')).toBe('500.0'); | ||
| expect(convertMicroToMacro('1234567.89', 'u')).toBe('1.23456789'); | ||
| expect(convertMicroToMacro('1000000.123456', 'm')).toBe('1000.000123456'); | ||
| expect(convertMicroToMacro('1', 'u')).toBe('0.000001'); | ||
| expect(convertMicroToMacro('0', 'u')).toBe('0.0'); | ||
| expect(convertMicroToMacro('0.0', 'u')).toBe('0.0'); | ||
| }); | ||
|
|
||
| test('converts macro to micro correctly', () => { | ||
| expect(convertMacroToMicro('1.000000', 'u')).toBe('1000000'); | ||
| expect(convertMacroToMicro('1.000000', 6)).toBe('1000000'); | ||
| expect(convertMacroToMicro('0.500000', 'm')).toBe('500'); | ||
| expect(convertMacroToMicro('1.23456789', 'u')).toBe('1234567.89'); | ||
| expect(convertMacroToMicro('1000.000123456', 'm')).toBe('1000000.123456'); | ||
| expect(convertMacroToMicro('1', 'u')).toBe('1000000'); | ||
| expect(convertMacroToMicro('0', 'u')).toBe('0'); | ||
| expect(convertMacroToMicro('0.0', 'u')).toBe('0'); | ||
| }); | ||
|
|
||
| test('formatAmountToInternational formats numbers correctly', () => { | ||
| expect(formatAmountToInternational(1234567.89, 2)).toBe('1,234,567.89'); | ||
| expect(formatAmountToInternational(1000, 0)).toBe('1,000'); | ||
| expect(formatAmountToInternational(1234.5678, 3)).toBe('1,234.567'); | ||
| }); | ||
|
|
||
| test('DENOM_EXPONENTS contains correct values', () => { | ||
| expect(DENOM_EXPONENTS).toEqual({ | ||
| 'd': 1, | ||
| 'c': 2, | ||
| 'm': 3, | ||
| 'u': 6, | ||
| 'n': 9, | ||
| 'p': 12, | ||
| 'f': 15, | ||
| 'a': 18, | ||
| 'z': 21, | ||
| 'y': 24, | ||
| }); | ||
| }); | ||
|
|
||
| test('getUnitsConfigFromDenom returns correct values', () => { | ||
| expect(getUnitsConfigFromDenom('inj')).toEqual({ units: 18, microDenom: 'inj', macroDenom: 'inj' }); | ||
| expect(getUnitsConfigFromDenom('uatom')).toEqual({ units: 6, microDenom: 'uatom', macroDenom: 'ATOM' }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Test coverage is comprehensive but missing validation for compareStringNumber function
The test suite provides good coverage for most utility functions, but it's missing tests for the compareStringNumber function which is part of the implementation. The current tests effectively validate:
- Micro to macro conversions with various units and edge cases
- Macro to micro conversions with different scenarios
- Amount formatting with internationalization
- Denom exponents constants
- Unit configuration retrieval
However, there are opportunities for improvement:
- Add test cases for
compareStringNumberfunction, including ascending and descending order scenarios - Add error cases for invalid unit types in conversion functions
- Include edge cases for very large numbers in formatting
🔗 Analysis chain
Well-structured test suite with good coverage.
The test suite provides comprehensive coverage of the core functionality for denomination utilities. While there are suggestions for additional test cases, the current implementation effectively validates the key requirements mentioned in the PR objectives, particularly ensuring correct conversions between macro and micro denominations.
Let's verify the test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any TODOs or FIXMEs in the denom implementation
rg -i "todo|fixme" "packages/andrjs/src/utils/denom.ts"
# Check for any error handling in the implementation
rg "throw|Error" "packages/andrjs/src/utils/denom.ts"
Length of output: 109
Script:
#!/bin/bash
# Let's first check the actual implementation file to understand what's being tested
cat packages/andrjs/src/utils/denom.ts
# Also check for any other test files related to denom
fd denom.spec -e ts -e js
Length of output: 4774
packages/andrjs/src/utils/denom.ts
Outdated
| const decimals = (amount.substring(amount.length - numZeroes)).concat(subDecimals.join()).replace(/0+$/, '').padEnd(1, '0'); | ||
| return fixed.concat('.').concat(decimals); |
There was a problem hiding this comment.
Add input validation and fix decimal handling.
The current implementation has a few potential issues:
- No validation for negative numbers or invalid input.
- Using
.join()without a separator could cause issues if there are multiple subDecimals.
Consider applying these improvements:
export const convertMicroToMacro = (amount: string, unit: UNITS) => {
+ if (!/^-?\d+\.?\d*$/.test(amount)) {
+ throw new Error('Invalid amount format');
+ }
const [realAmount, ...subDecimals] = amount.split('.');
- amount = realAmount;
+ const isNegative = realAmount.startsWith('-');
+ amount = isNegative ? realAmount.slice(1) : realAmount;
let numZeroes = typeof unit === 'number' ? unit : DENOM_EXPONENTS[unit];
amount = amount.padStart(numZeroes + 1, '0');
const fixed = amount.substring(0, amount.length - numZeroes);
- const decimals = (amount.substring(amount.length - numZeroes)).concat(subDecimals.join()).replace(/0+$/, '').padEnd(1, '0');
- return fixed.concat('.').concat(decimals);
+ const decimals = (amount.substring(amount.length - numZeroes))
+ .concat(subDecimals.join(''))
+ .replace(/0+$/, '')
+ .padEnd(1, '0');
+ return `${isNegative ? '-' : ''}${fixed}.${decimals}`;📝 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.
| const decimals = (amount.substring(amount.length - numZeroes)).concat(subDecimals.join()).replace(/0+$/, '').padEnd(1, '0'); | |
| return fixed.concat('.').concat(decimals); | |
| export const convertMicroToMacro = (amount: string, unit: UNITS) => { | |
| if (!/^-?\d+\.?\d*$/.test(amount)) { | |
| throw new Error('Invalid amount format'); | |
| } | |
| const [realAmount, ...subDecimals] = amount.split('.'); | |
| const isNegative = realAmount.startsWith('-'); | |
| amount = isNegative ? realAmount.slice(1) : realAmount; | |
| let numZeroes = typeof unit === 'number' ? unit : DENOM_EXPONENTS[unit]; | |
| amount = amount.padStart(numZeroes + 1, '0'); | |
| const fixed = amount.substring(0, amount.length - numZeroes); | |
| const decimals = (amount.substring(amount.length - numZeroes)) | |
| .concat(subDecimals.join('')) | |
| .replace(/0+$/, '') | |
| .padEnd(1, '0'); | |
| return `${isNegative ? '-' : ''}${fixed}.${decimals}`; |
packages/cli/src/handlers/wasm.ts
Outdated
| const encodedKey = !!flags.encoded; | ||
| const keyBytes = encodedKey ? Uint8Array.from(Buffer.from(key, 'hex')) : Uint8Array.from(Buffer.from(key, 'utf8')); |
There was a problem hiding this comment.
Add error handling for key parsing
When the encoded flag is set, parsing the key with Buffer.from(key, 'hex') may throw an exception if the key is not valid hex. Adding error handling will provide a user-friendly error message and prevent the program from crashing.
Apply this diff to add error handling:
const encodedKey = !!flags.encoded;
- const keyBytes = encodedKey ? Uint8Array.from(Buffer.from(key, 'hex')) : Uint8Array.from(Buffer.from(key, 'utf8'));
+ let keyBytes;
+ try {
+ keyBytes = encodedKey
+ ? Uint8Array.from(Buffer.from(key, 'hex'))
+ : Uint8Array.from(Buffer.from(key, 'utf8'));
+ } catch (error) {
+ console.log();
+ console.log(pc.red("Invalid key format."));
+ return;
+ }📝 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.
| const encodedKey = !!flags.encoded; | |
| const keyBytes = encodedKey ? Uint8Array.from(Buffer.from(key, 'hex')) : Uint8Array.from(Buffer.from(key, 'utf8')); | |
| const encodedKey = !!flags.encoded; | |
| let keyBytes; | |
| try { | |
| keyBytes = encodedKey | |
| ? Uint8Array.from(Buffer.from(key, 'hex')) | |
| : Uint8Array.from(Buffer.from(key, 'utf8')); | |
| } catch (error) { | |
| console.log(); | |
| console.log(pc.red("Invalid key format.")); | |
| return; | |
| } |
packages/cli/src/handlers/wasm.ts
Outdated
| { | ||
| requestMessage: "Input Key:", | ||
| }, |
There was a problem hiding this comment.
Add Input Validation for 'Input Key'
The 'Input Key' prompt currently lacks input validation. To ensure the key is provided in the correct format and to prevent potential errors during execution, it's recommended to add a validation function.
Apply this diff to add input validation:
{
requestMessage: "Input Key:",
+ validate: (input: string) => {
+ if (input.length === 0) {
+ console.log();
+ console.log(pc.red("Key cannot be empty."));
+ return false;
+ }
+ return true;
+ },
},📝 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.
| { | |
| requestMessage: "Input Key:", | |
| }, | |
| { | |
| requestMessage: "Input Key:", | |
| validate: (input: string) => { | |
| if (input.length === 0) { | |
| console.log(); | |
| console.log(pc.red("Key cannot be empty.")); | |
| return false; | |
| } | |
| return true; | |
| }, | |
| }, |
There was a problem hiding this comment.
@daniel-wehbe has provided a verbal confirmation on testing. Should be good to move forward with the release.
Motivation
wasm contract-statecommandSummary by CodeRabbit
Release Notes
New Features
"wild-oranges-give"for improved package management.query-rawandcontract-statefor enhanced contract querying capabilities.Improvements
Bug Fixes
Testing