Fix resolve implementation #8413
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 6cd681c The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
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. |
Contributor
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
MananTank
approved these changes
Nov 13, 2025
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/thirdweb/src/utils/bytecode/resolveImplementation.ts (1)
59-62: LGTM! Concurrent resolution improves performance.Fetching both implementation sources in parallel is a good optimization. However, note that this introduces an implicit priority: storage-slot implementations are checked before contract-call implementations since they appear first in the array.
Consider adding a comment documenting this priority order:
+ // Fetch implementations from both sources concurrently + // Storage slot implementations are checked first, then contract call implementations const implementations = await Promise.all([ getImplementationFromStorageSlot(contract), getImplementationFromContractCall(contract), ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/afraid-regions-push.md(1 hunks)packages/thirdweb/src/utils/bytecode/resolveImplementation.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-13T13:03:41.732Z
Learnt from: MananTank
Repo: thirdweb-dev/js PR: 7332
File: apps/dashboard/src/app/(app)/(dashboard)/(chain)/[chain_id]/[contractAddress]/public-pages/nft/overview/nft-drop-claim.tsx:82-90
Timestamp: 2025-06-13T13:03:41.732Z
Learning: The thirdweb `contract` object is serializable and can safely be used in contexts (e.g., React-Query keys) that require serializable values.
Applied to files:
.changeset/afraid-regions-push.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Unit Tests
- GitHub Check: Build Packages
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
.changeset/afraid-regions-push.md (1)
1-5: LGTM!The changeset follows the standard format and appropriately documents this fix as a patch-level change.
packages/thirdweb/src/utils/bytecode/resolveImplementation.ts (1)
89-89: LGTM! Appropriate fallback behavior.Returning the original contract address and bytecode when no valid implementation is found is the correct fallback behavior.
Contributor
size-limit report 📦
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8413 +/- ##
==========================================
+ Coverage 54.83% 54.84% +0.01%
==========================================
Files 919 919
Lines 60844 60846 +2
Branches 4134 4141 +7
==========================================
+ Hits 33364 33372 +8
+ Misses 27378 27372 -6
Partials 102 102
🚀 New features to boost your workflow:
|
Contributor
Merge activity
|
<!--
## title your PR with this format: "[SDK/Dashboard/Portal] Feature/Fix: Concise title for the changes"
If you did not copy the branch name from Linear, paste the issue tag here (format is TEAM-0000):
## Notes for the reviewer
Anything important to call out? Be sure to also clarify these in your comments.
## How to test
Unit tests, playground, etc.
-->
<!-- start pr-codex -->
---
## PR-Codex overview
This PR focuses on improving the `resolveImplementation` function in the `thirdweb` package by enhancing the implementation resolution process for proxy contracts.
### Detailed summary
- Replaced the existing implementation resolution logic with a more robust approach using `Promise.all` to fetch implementations from both storage and contract calls.
- Introduced a loop to handle multiple implementations, ensuring that valid addresses are checked before proceeding.
- Updated the return logic to ensure correct addresses and bytecode are returned based on the resolved implementation.
> ✨ Ask PR-Codex anything about this PR by commenting with `/codex {your question}`
<!-- end pr-codex -->
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved the reliability of contract implementation resolution to handle edge cases more robustly when detecting implementations across proxy contracts and various contract architecture types. Processing is now more efficient through concurrent implementation detection and optimized fallback handling, ensuring more consistent and faster results in complex contract scenarios.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
graphite-app
bot
force-pushed
the
yash/resolve-implementation-fix
branch
from
c20c38a to
6cd681c
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR-Codex overview
This PR focuses on improving the
resolveImplementationfunction in thethirdwebpackage by enhancing the way implementation addresses are resolved from both storage slots and contract calls, ensuring more robust handling of proxy types.Detailed summary
Promise.allfor concurrent fetching of implementation addresses.beaconand streamlined implementation address retrieval.Summary by CodeRabbit