fix: additional named addresss by base64 instead of utf8 in cString convertion #23
Conversation
djm07073
changed the title
WalkthroughThe changes in this pull request include updates to the Changes
Poem
🪧 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 (3)
test/build.spec.ts (2)
21-22: Consider adding test cases for invalid addressesThe test includes both hex and bech32 address formats, which is good. Consider adding test cases for:
- Invalid hex addresses
- Invalid bech32 addresses
- Address format conversion edge cases
Would you like me to help generate these additional test cases?
106-106: Consider using AccAddress for byte array representationThe hardcoded byte array for addresses could be replaced with
AccAddressutilities for better maintainability.-'{"address":[0,0,0,0,0,0,0,0,0,0,0,0,24,206,215,65,14,243,151,179,70,51,139,15,192,111,122,220,19,141,191,72],"name":"dummy"}' +'{"address":${AccAddress.toBytes('0x18ced7410ef397b346338b0fc06f7adc138dbf48')},"name":"dummy"}'Also applies to: 115-116
src/builder.ts (1)
53-54: Consider documenting the encoding changesThe switch from UTF-8 to Base64 encoding and the changes to address handling are significant architectural changes. Consider:
- Adding comments explaining why Base64 encoding is now required
- Updating the class documentation to reflect the new address handling behavior
- Ensuring these changes are documented in the migration guide if this is a breaking change
Also applies to: 65-66, 200-200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.json(2 hunks)src/builder.ts(3 hunks)src/types/bcs.ts(2 hunks)test/build.spec.ts(4 hunks)test/contract/dummy/Move.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/contract/dummy/Move.toml
🔇 Additional comments (6)
package.json (1)
59-59: Verify compatibility with @initia/initia.js version
The change from @mysten/bcs to @initia/initia.js aligns with the PR objectives. However, let's verify if version ^0.2.23 is the latest stable version and check for any breaking changes.
✅ Verification successful
Version 0.2.23 of @initia/initia.js is the latest stable version
The package.json specifies version ^0.2.23 which matches the latest stable version available on npm registry. This indicates the dependency is up-to-date and compatible.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check latest version and changelog
curl -s https://registry.npmjs.org/@initia/initia.js | jq -r '.["dist-tags"].latest'
Length of output: 93
src/types/bcs.ts (2)
1-1: LGTM: Import change aligns with dependency update
The import change from @mysten/bcs to @initia/initia.js correctly reflects the dependency update in package.json.
16-16: Verify address type compatibility
The change from bcs.bytes(32) to bcs.address() appears to support both bech32 and hex address formats. Let's verify the implementation in the new dependency.
test/build.spec.ts (2)
4-4: LGTM: Added support for new address format
The import of AccAddress and addition of test2ModuleAddr properly supports the new bech32 address format.
Also applies to: 8-8
40-99: Verify address conversion in module decoding
The test cases verify module decoding for both address formats. Let's ensure the conversion is consistent across the codebase.
✅ Verification successful
Let me gather more information about the address conversion implementation and usage.
Based on the gathered information, I can now generate the final response:
Address conversion in module decoding is consistent
The test cases correctly validate both address formats:
- Raw hex address (
0x18ced7410ef397b346338b0fc06f7adc138dbf48) - Converted address using
AccAddress.toHex()
The decode_module_bytes implementation in src/builder.ts handles the bytecode decoding uniformly, and the address format conversion is only used in test assertions, not in the core functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other address conversions
rg "AccAddress\.toHex|\.decode_module_bytes" --type ts
Length of output: 366
Script:
#!/bin/bash
# Check the implementation of decode_module_bytes and related code
ast-grep --pattern 'decode_module_bytes($$$)'
# Look for AccAddress usage patterns
rg "AccAddress" -A 3 --type ts
# Check the builder implementation
cat src/builder.ts
Length of output: 9463
src/builder.ts (1)
65-66: Verify Base64 encoding changes
The switch from UTF-8 to Base64 encoding aligns with the PR objectives to fix UTF-8 handling issues. However, we should verify:
- All relevant Buffer conversions are updated consistently
- The receiving functions (in
libcompilerandlibmovevm) expect Base64 encoded data
Also applies to: 200-200
✅ Verification successful
Let me analyze the results and generate additional verification scripts to check the receiving functions.
This will help us verify if the receiving functions are properly handling Base64 encoded data.
Let's check the test files and vm.ts to understand how the Base64 encoding is handled.
Base64 encoding changes are properly implemented
The verification shows that:
- Base64 encoding is consistently used for binary data (compiler payload, test options, module bytes, script bytes) that needs to be passed to native functions
- UTF-8 encoding is correctly retained for string-based metadata (package names, move versions)
- The receiving functions in the native layer properly handle Base64 encoded data, as evidenced by the successful encoding/decoding tests in
build.spec.tsandscript.spec.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for remaining UTF-8 encoding usage in buffer operations
echo "Searching for remaining UTF-8 encoding in buffer operations..."
rg "toString\('utf-8'\)" --type ts
rg "Buffer\.from.*'utf-8'" --type ts
# Search for Base64 encoding usage
echo "Searching for Base64 encoding usage..."
rg "toString\('base64'\)" --type ts
rg "Buffer\.from.*'base64'" --type ts
# Search for buffer conversion patterns that might need updating
echo "Searching for potential missing conversions..."
rg "ref\.allocCString\([^,]+\)" --type ts
Length of output: 1539
Script:
#!/bin/bash
# Search for functions that handle the Base64 encoded data
echo "Searching for Base64 decoding in libcompiler and libmovevm..."
fd -e rs -e cpp -e h | xargs rg -l "base64|decode" || true
# Search for function signatures that receive the encoded data
echo "Searching for relevant function signatures..."
ast-grep --pattern 'fn $_($$$) -> $$$' | rg "compile|build|execute" || true
# Check if there are any test files that verify encoding
echo "Searching for encoding-related tests..."
fd -e test.ts -e spec.ts | xargs rg -l "base64|encode|decode" || true
Length of output: 651
Script:
#!/bin/bash
# Check test files for Base64 encoding verification
echo "Checking test files for Base64 encoding usage..."
rg -A 5 "base64|encode|decode" test/build.spec.ts test/script.spec.ts
# Check vm.ts for how it handles the encoded data
echo "Checking vm.ts for Base64 handling..."
rg -A 5 "base64|encode|decode" src/lib/vm.ts
# Look for any remaining UTF-8 to Base64 conversion patterns
echo "Checking for potential conversion patterns..."
rg "Buffer\.(from|to)" src/builder.ts -A 2
Length of output: 2949
| additional_named_addresses: | ||
| this.buildOptions.addtionalNamedAddresses || [], |
There was a problem hiding this comment.
💡 Codebase verification
Fix typo in property name addtionalNamedAddresses
The property name has a typo and is consistently used incorrectly across the codebase:
src/types/options.ts: Type definitionsrc/builder.ts: Property usagetest/build.spec.ts: Test case
🔗 Analysis chain
Fix typo in property name and verify address handling
There's a typo in the property name: addtionalNamedAddresses should be additionalNamedAddresses.
Additionally, since the address transformation logic has been removed, we should verify that the addresses are properly handled elsewhere in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for correct and incorrect property name usage
echo "Searching for property name usage..."
rg "addtionalNamedAddresses" --type ts
rg "additionalNamedAddresses" --type ts
# Search for address transformation logic
echo "Searching for address handling..."
rg -A 5 "additional_named_addresses" --type ts
Length of output: 971
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
.github/workflows/test.yml(3 hunks)
🔇 Additional comments (1)
.github/workflows/test.yml (1)
35-39: LGTM! Good CI/CD practice with explicit Node.js setup
The addition of an explicit Node.js setup step with version pinning and npm caching is a good practice that:
- Ensures consistent Node.js version across builds
- Improves CI performance through npm caching
- Makes the build environment more explicit and maintainable
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/test.yml (1)
35-39: Consider pinning to a specific Node.js versionWhile using Node.js 20 is good, it's recommended to pin to a specific version (e.g., '20.x' or '20.10.0') for better reproducibility and to avoid potential breaking changes.
- node-version: '20' + node-version: '20.x'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/test.yml(3 hunks)
🔇 Additional comments (1)
.github/workflows/test.yml (1)
8-8: Fix the file pattern syntax for libmovevm paths
The pattern libmovevm.** uses incorrect syntax for GitHub Actions path filtering. Use libmovevm/** for proper directory matching.
Also applies to: 18-18
|
fix in other pr. github actions error out in npm install |
Feat
Fix
Test
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores