fix: use base64 encode instead of utf8 in cString #24
Conversation
|
Warning Rate limit exceeded@djm07073 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 35 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. WalkthroughThe changes in this pull request include updates to the GitHub Actions workflow for setting up Node.js, modifications 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 (1)
test/build.spec.ts (1)
Line range hint
111-126: Remove duplicate test caseThis test case is a duplicate of the previous 'reads module info correctly' test (lines 100-108). Consider removing this duplicate or renaming it if it serves a different purpose.
- it('reads module info correctly', async () => { - const binary = await builder.get('dummy') - const moduleInfo = await MoveBuilder.read_module_info(binary) - const expectedModuleInfo = JSON.stringify({ - address: [ - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 4, - ], - name: 'dummy', - }) - - expect(moduleInfo).toEqual(expectedModuleInfo) - })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/test.yml(1 hunks)src/builder.ts(2 hunks)test/build.spec.ts(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test.yml
35-35: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (4)
src/builder.ts (3)
74-75: LGTM: Appropriate use of base64 encoding for binary data
The switch from UTF-8 to base64 encoding for compiler payload bytes is correct, as base64 is designed for encoding binary data without data loss.
209-209: LGTM: Consistent encoding approach
The test options bytes are now correctly encoded using base64, maintaining consistency with the encoding approach used in other methods.
Line range hint 1-300: Verify comprehensive encoding updates
Let's ensure all binary data handling methods have been updated to use base64 encoding consistently.
✅ Verification successful
Based on the search results, I can now generate the final response:
Binary data handling is correctly implemented
The codebase shows consistent encoding practices:
- Base64 encoding is correctly used for all binary data transfers (
compilerPayloadBytes,testOptBytes,moduleBytes,scriptBytes,compiledBinary) - UTF-8 encoding is only used for text data (package names, version strings) and reading text files (Move.toml)
- No instances of ASCII encoding were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining UTF-8 encoding usage in binary data handling
rg -i "toString\('utf-?8'\)" --type ts
# Search for base64 encoding usage to verify consistency
rg -i "toString\('base64'\)" --type ts
# Look for potential Buffer.from operations that might need updating
rg "Buffer\.from\([^,]+,\s*['\"](utf-?8|ascii)['\"]" --type ts
Length of output: 912
test/build.spec.ts (1)
38-96: LGTM: Comprehensive test coverage for base64 encoding
The test cases properly validate the module bytes decoding with both 'dummy' and 'hihi' modules, ensuring the base64 encoding changes work correctly.
Fix
Test
Summary by CodeRabbit
New Features
Bug Fixes
Tests