fix: handle short storage values in beacon proxy pattern #7282
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: e493133 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded@catalyst17 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 5 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)
WalkthroughA new test suite has been added to verify the handling of the beacon proxy pattern in smart contract bytecode resolution. The test specifically targets scenarios involving short storage values and ensures the implementation address is correctly resolved without errors, using mocked dependencies and conditional execution based on an environment variable. Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant MockedGetBytecode
participant MockedRpcClient
participant ResolveImplementation
TestSuite->>MockedGetBytecode: getBytecode (returns empty, then dummy bytecode)
TestSuite->>MockedRpcClient: eth_getStorageAt (returns short hex string)
TestSuite->>ResolveImplementation: resolveImplementation(address, client)
ResolveImplementation->>MockedGetBytecode: getBytecode(address)
ResolveImplementation->>MockedRpcClient: eth_getStorageAt(address, slot)
ResolveImplementation-->>TestSuite: Returns implementation address
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/thirdweb/src/utils/bytecode/handleBeaconProxyPattern.test.ts (1)
1-26: Remove unused imports to improve code clarity.Multiple imports are flagged as unused by static analysis and should be removed to keep the code clean and maintainable.
Apply this diff to remove unused imports:
-import type { Abi } from "abitype"; import type { EIP1193RequestFn, EIP1474Methods } from "viem"; import { beforeEach, describe, expect, it, vi } from "vitest"; -import { - DUMMY_BYTECODE, - ERC1967_PROXY_BYTECODE, - ERC1967_PROXY_CONSTRUCTOR_ABI, -} from "../../../test/src/abis/proxy.js"; import { ANVIL_CHAIN } from "../../../test/src/chains.js"; import { TEST_CLIENT } from "../../../test/src/test-clients.js"; -import { - BASE_USDC_IMPLEMENTATION, - BASE_USDC_PROXY_CONTRACT, - NFT_DROP_CONTRACT, - NFT_DROP_IMPLEMENTATION, - POLYGON_USDT_IMPLEMENTATION, - POLYGON_USDT_PROXY_CONTRACT, -} from "../../../test/src/test-contracts.js"; -import { TEST_ACCOUNT_A } from "../../../test/src/test-wallets.js"; import { getBytecode } from "../../contract/actions/get-bytecode.js"; -import { getContract } from "../../contract/contract.js"; -import { deployContract } from "../../contract/deployment/deploy-with-abi.js"; import { eth_getStorageAt } from "../../rpc/actions/eth_getStorageAt.js"; import { getRpcClient } from "../../rpc/rpc.js"; import { resolveImplementation } from "./resolveImplementation.js";🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.(lint/correctness/noUnusedImports)
[error] 5-5: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.(lint/correctness/noUnusedImports)
[error] 6-6: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.(lint/correctness/noUnusedImports)
[error] 7-7: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.(lint/correctness/noUnusedImports)
[error] 12-12: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.(lint/correctness/noUnusedImports)
[error] 13-13: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.(lint/correctness/noUnusedImports)
[error] 14-14: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.(lint/correctness/noUnusedImports)
[error] 15-15: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.(lint/correctness/noUnusedImports)
[error] 16-16: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.(lint/correctness/noUnusedImports)
[error] 17-17: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.(lint/correctness/noUnusedImports)
[error] 19-19: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.(lint/correctness/noUnusedImports)
[error] 21-21: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.(lint/correctness/noUnusedImports)
[error] 22-22: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.(lint/correctness/noUnusedImports)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/thirdweb/src/utils/bytecode/handleBeaconProxyPattern.test.ts(1 hunks)packages/thirdweb/src/utils/bytecode/resolveImplementation.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/thirdweb/src/utils/bytecode/resolveImplementation.ts (1)
packages/thirdweb/src/exports/thirdweb.ts (1)
isAddress(308-308)
packages/thirdweb/src/utils/bytecode/handleBeaconProxyPattern.test.ts (1)
packages/thirdweb/src/utils/bytecode/resolveImplementation.ts (1)
resolveImplementation(26-90)
🪛 Biome (1.9.4)
packages/thirdweb/src/utils/bytecode/handleBeaconProxyPattern.test.ts
[error] 1-1: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 5-5: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 6-6: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 7-7: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 12-12: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 13-13: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 14-14: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 15-15: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 16-16: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 17-17: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 19-19: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 21-21: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 22-22: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Unit Tests
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Size
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
packages/thirdweb/src/utils/bytecode/resolveImplementation.ts (1)
52-52: Excellent defensive programming practice!Adding the
isAddress(beacon)validation before proceeding with beacon proxy resolution is a smart improvement. This prevents potential runtime errors when the storage data doesn't contain a valid Ethereum address format, making the code more robust against edge cases with malformed storage values.packages/thirdweb/src/utils/bytecode/handleBeaconProxyPattern.test.ts (2)
38-67: Well-structured test for the edge case!The test effectively verifies the handling of short storage values in beacon proxy pattern resolution. The mocking strategy correctly simulates the scenario where
eth_getStorageAtreturns a short value ("0x1234"), ensuring the implementation doesn't throw errors and handles this edge case gracefully.
32-34: Appropriate use of conditional test execution.The conditional test execution based on
TW_SECRET_KEYenvironment variable is a good practice for tests that may require specific setup or credentials, ensuring they only run in appropriate environments.
size-limit report 📦
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7282 +/- ##
==========================================
- Coverage 55.58% 55.58% -0.01%
==========================================
Files 909 909
Lines 58670 58670
Branches 4163 4158 -5
==========================================
- Hits 32613 32609 -4
- Misses 25950 25955 +5
+ Partials 107 106 -1
🚀 New features to boost your workflow:
|
catalyst17
force-pushed
the
fix_handle_short_storage_values_in_beacon_proxy_pattern
branch
2 times, most recently
from
7286123 to
b55d4cf
Compare
catalyst17
force-pushed
the
fix_handle_short_storage_values_in_beacon_proxy_pattern
branch
from
b55d4cf to
e493133
Compare
joaquim-verges
deleted the
fix_handle_short_storage_values_in_beacon_proxy_pattern
branch
[SDK] Fix: Handle short storage values in beacon proxy pattern
Notes for the reviewer
Tests follow-up to #7281
Added a unit test that specifically verifies the implementation can handle short storage values returned from
eth_getStorageAt, which was the original issue we were trying to fix.Summary by CodeRabbit
PR-Codex overview
This PR focuses on fixing the implementation resolution for the Beacon proxy pattern in the
thirdwebpackage by enhancing the test coverage to ensure proper handling of short storage values.Detailed summary
handleBeaconProxyPattern.test.tsto verify handling of short storage values.getBytecode,eth_getStorageAt, andgetRpcClient.