Skip to content
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

Support video avatars #3700

Merged
merged 8 commits into from
Dec 18, 2023
Merged

Support video avatars #3700

merged 8 commits into from
Dec 18, 2023

Conversation

xpaczka
Copy link
Contributor

@xpaczka xpaczka commented Dec 14, 2023

Resolves #3688

What has been done

Added video support for wallet avatars

Testing

  • Test with video avatar (example user 0xab7375daf14bc661a0c9aff8800d49a34b037a98)
  • Test with image avatar

Latest build: extension-builds-3700 (as of Mon, 18 Dec 2023 14:56:44 GMT).

@xpaczka xpaczka marked this pull request as ready for review December 14, 2023 10:52
@michalinacienciala
Copy link
Contributor

Looks that something is off about the default doggos avatars - they look zoomed in:
image

@@ -66,6 +66,7 @@ export type AccountData = {
ens: {
name?: DomainName
avatarURL?: URI
avatarType?: string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: This is changing shape of the redux and in the extension we are persisting redux state between reloads. Usually we are adding redux migration but in this case I think we are fine because we are ok with this field being undefined. Anyway - I'll test the migration to make sure it is ok.

EDIT:
ok so we will need a migration unfortunately
Reproduction steps:

  • checkout main
  • add read-only account with avatar - expected: no avatar is loaded (not even the default one)
  • checkout this branch
  • reload extension background script

result: no avatar at all, not even the light green background
expected: there is a video avatar visible

image

@michalinacienciala
Copy link
Contributor

The failures in the tests are caused by a known issue: #3675.
In the tests we're first mark BANANA token as trusted and later remove it from the token list. The test expects to not see the BANANA token on the list after that, but due to the bug the token reappears on the list.
This may not always be failing, as sometimes it takes longer and sometimes shorter for the deleted asset to reappear.

ens: {
name?: DomainName
avatarURL?: URI
avatarType?: string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

old state shouldn't have avatarType because this is actually the field we just added in this PR right?


return {
...typedPrevState,
assets: [],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we are resetting assets?

if (!avatarURL) {
accountData.avatarType = undefined
} else {
const fileTypeResponse = await fetch(avatarURL, { method: "HEAD" })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I don't think this will work correctly because we are not waiting for results here at all, script will continue sync execution and will deserialize the preloaded state without waiting for these fetches. Actually, now I'm even thinking that we shouldn't even do it here at all - migrations should be working fast and be deterministic 🤔 Sorry about making it more confusing than it should be 😓
Can we try to load the avatarType when the actual avatar is being loaded on the frontend side?

Copy link
Contributor

@jagodarybacka jagodarybacka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks 🔥
image

@jagodarybacka jagodarybacka merged commit bb4824c into main Dec 18, 2023
6 checks passed
@jagodarybacka jagodarybacka deleted the video-avatar branch December 18, 2023 14:48
@xpaczka xpaczka mentioned this pull request Dec 27, 2023
xpaczka added a commit that referenced this pull request Dec 29, 2023
## What's Changed
* Enable swaps on Sepolia testnet by @michalinacienciala in
#3710
* Prevent read-only address submission when validating input by @xpaczka
in #3705
* Support video avatars by @xpaczka in
#3700
* Replace `customStyle` property with `style` by @xpaczka in
#3715
* Show tooltips on hovering over icons with no labels by @xpaczka in
#3711
* Minor UI issues by @xpaczka in
#3716
* Fix hiding unverified assets by @jagodarybacka in
#3708
* v0.54.0 by @xpaczka in
#3706
* chore(ui): typo fix by @IssouChancla in
#3717
* Switch e2e tests run on Goerli to Sepolia by @michalinacienciala in
#3719

## New Contributors
* @xpaczka made their first contribution in
#3705
* @IssouChancla made their first contribution in
#3717

**Full Changelog**:
v0.54.0...v0.55.0

Latest build:
[extension-builds-3720](https://github.com/tahowallet/extension/suites/19343332470/artifacts/1136165985)
(as of Wed, 27 Dec 2023 09:28:52 GMT).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support video user avatars
3 participants