-
Notifications
You must be signed in to change notification settings - Fork 570
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update snap_getPreferences
#3093
base: main
Are you sure you want to change the base?
Conversation
*/ | ||
export type GetPreferencesResult = { | ||
locale: string; | ||
currency: string; | ||
hideBalances: boolean; | ||
useSecurityAlerts: boolean; | ||
useSimulations: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useSimulations: boolean; | |
useTransactionSimulations: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be signatures as well, so that's why I kept it generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I feel like "simulations" is too generic, but I'm not sure if there's a good name for both 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about simulateOnChainActions
?
@@ -13,9 +13,23 @@ export type GetPreferencesParams = never; | |||
* @property locale - The user's selected locale. | |||
* @property currency - The user's selected currency. | |||
* @property hideBalances - Whether the user has chosen to hide balances. | |||
* @property useSecurityAlerts - Whether to run transactions and signatures through blockaid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this relevant for Snaps? They can't hook into Blockaid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have no idea, maybe Solana team has plans to use blockaid? cc: @Montoya
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup they are integrating blockaid's API into the snap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should make it more generic and not mention any providers, our Snaps may choose to use Blockaid, but other Snaps could use other providers for "security alerts"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the description.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3093 +/- ##
=======================================
Coverage 94.88% 94.88%
=======================================
Files 506 506
Lines 11123 11123
Branches 1709 1709
=======================================
Hits 10554 10554
Misses 569 569 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR doesn't update snap-simulation, can we update it ?
Yes please, we can just return true for all the new flags tbh |
Done |
Updating
snap_getPreferences
return type to include additional properties.Closes #3076