-
Notifications
You must be signed in to change notification settings - Fork 394
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
Underbridge: Drop updated dApp connections feature flag, further Taho renames #3595
Conversation
27e663c
to
09c095a
Compare
It is enabled and shall remain so!
Anything that is public API is duplicated as tally and taho, everything else is renamed. Some string constants still say tally_ as checking for breakage requires some extra diligence.:
09c095a
to
96b805a
Compare
Did some light testing here with dApp connections across a few places, including stake.lido.fi which has direct Taho support. All working well, so going to ship it. |
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.
Code looks good, just one minor comment
providers: dedupedProviders, | ||
shouldSetTallyForCurrentProvider( | ||
shouldSetTally: boolean, | ||
shouldReload = false | ||
) { | ||
if (shouldSetTally && this.currentProvider !== this.tallyProvider) { | ||
this.currentProvider = this.tallyProvider | ||
this.shouldSetTahoForCurrentProvider(shouldSetTally, shouldReload) |
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.
Do we have to keep shouldSetTallyForCurrentProvider
as an alias?
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 went back and forth on this one--it was designed for internal calls, and I'm not sure why a third party would call it. But, it is strictly speaking public API, because it needed to be for us to access it in the first place. I felt safe nixing it since the integrations that do exist are provider-style and wouldn't need this call...
With that said, if you'd feel more comfortable with an alias sticking around, I'll slip it in.
## Highlights - Further performance improvements. - Groundwork to rename backend dApp connection things (for the provider) to Taho naming. ## What's Changed * v0.46.1 by @Shadowfiend in #3598 * Updates to release checklist: Removed Mintkudos test by @andreachapman in #3600 * Revert "Revert "Debounce dispatched state diffs"" by @hyphenized in #3579 * Underbridge: Drop updated dApp connections feature flag, further Taho renames by @Shadowfiend in #3595 **Full Changelog**: v0.46.1...v0.47.0 Latest build: [extension-builds-3601](https://github.com/tahowallet/extension/suites/15160003504/artifacts/866327987) (as of Wed, 16 Aug 2023 19:09:51 GMT).
This PR does two things:
ENABLE_UPDATED_DAPP_CONNECTIONS
feature flag completely, as well as all related code paths.tally
in the window provider to saytaho
instead, except if they were part of a publicly-visible API. In the latter case, adds ataho
equivalent, so that we can phasetally
out at an unspecified later date.Latest build: extension-builds-3595 (as of Tue, 15 Aug 2023 20:01:43 GMT).