[SDK] Fix: Throw when enclave migration fails #5473
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Your org has enabled the Graphite merge queue for merging into mainAdd the label “merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| console.error( | ||
| "Failed to migrate from sharded to enclave wallet, continuing with sharded wallet", | ||
| ); | ||
| throw new Error("Something went wrong logging into your wallet"); |
There was a problem hiding this comment.
There appears to be a logical contradiction between the console message and the error handling. The message states "continuing with sharded wallet", but the subsequent throw statement prevents any continuation. Consider either:
- Removing the
throwto allow fallback to sharded wallet as indicated - Updating the console message to accurately reflect that migration failure is a fatal error
The current implementation could mislead developers debugging wallet migration issues.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
worried about this because we still see some errors on DD about some users not being migrated - can we double check that first?
There was a problem hiding this comment.
There's more context on this here: https://thirdwebdev.slack.com/archives/C081B62FQ2K/p1732130602707349
Firekeeper and Jake wanted any migration failure to immediately enter a failed state and not roll over to sharded wallets
size-limit report 📦
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5473 +/- ##
=======================================
Coverage 43.88% 43.88%
=======================================
Files 1075 1075
Lines 55944 55943 -1
Branches 3909 3910 +1
=======================================
Hits 24550 24550
+ Misses 30711 30710 -1
Partials 683 683
*This pull request uses carry forward flags. Click here to find out more.
|
gregfromstl
added
the
merge-queue
Merge activity
|
gregfromstl
force-pushed
the
greg/cnct-2422-throw-if-sharded-migration-fails
branch
from
c191890 to
5a5673e
Compare
|
This PR has been inactive for 30 days. It is now marked as stale and will be closed in 5 days if no further activity occurs. |
|
Open |
|
This PR has been inactive for 30 days. It is now marked as stale and will be closed in 5 days if no further activity occurs. |
CNCT-2422
PR-Codex overview
This PR focuses on improving error handling during the migration from sharded to enclave wallets in the
native-connector.tsandweb-connector.tsfiles. It replacesconsole.warnstatements withconsole.errorand throws an error to better inform users of issues during the login process.Detailed summary
In
native-connector.ts:console.warnwithconsole.errorfor migration failure.throw new Errorstatement for better error handling.In
web-connector.ts:console.warnwithconsole.errorfor migration failure.throw new Errorstatement for better error handling.