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

fix(community): Missing community owner token image #16134

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

friofry
Copy link
Contributor

@friofry friofry commented Aug 15, 2024

What does the PR do

(!) suboptimal solution

This issue is a regression after refactoring and sending images as localhost URLs instead of base64 (which is good)
We try to create a token and pass URL as base64 image, SaveCommunityToken(protocol/communities/manager.go) fails to read it.

To keep the old format, I'm fetching image dataUrl (base64) and passing it to the API in the expected format

Affected areas

owner/master token

Architecture compliance

I admit the solution is not perfect. Ideally we should update the approach and just pass the community ID and get the image from the database. Not sure how to crop it properly though. And also update status-go (DeploymentParameters and DeployOwnerToken API endpoint)

  • I am familiar with the application architecture and agreed good practices.
    My PR is consistent with this document: Architecture guidelines

Screenshot of functionality (including design for comparison)

  • I've checked the design and this PR matches it
collectible_img.mp4

Impact on end user

Should fix missing collectible image

How to test

Create a community and mint an owner token, make sure the image is not empty

Risk

(since minting requires paying fees)

  • High risk: QA team MUST perform additional testing in the specified affected areas before merging.

fixes #15855

@@ -983,4 +983,49 @@ QtObject {
// #15331
return true
}

function toBase64(buffer) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I cannot useQt.btoa, because it produces a wrong result (different from js window.btoa).

Copy link
Member

Choose a reason for hiding this comment

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

Hmm.. I wonder how come, this should be identical

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why, but it just produces a different length result.
I've written some tests (in html and qml). And toBase64 works similar to btoa.

@status-im-auto
Copy link
Member

status-im-auto commented Aug 15, 2024

Jenkins Builds

Click to see older builds (7)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 48d51ca #1 2024-08-15 22:43:22 ~6 min tests/nim 📄log
✔️ 48d51ca #1 2024-08-15 22:46:32 ~10 min macos/aarch64 🍎dmg
✔️ 48d51ca #1 2024-08-15 22:47:50 ~11 min macos/x86_64 🍎dmg
48d51ca #1 2024-08-15 22:49:48 ~13 min tests/ui 📄log
✔️ 48d51ca #1 2024-08-15 22:52:04 ~15 min linux-nix/x86_64 📦tgz
✔️ 48d51ca #1 2024-08-15 22:53:56 ~17 min linux/x86_64 📦tgz
✔️ 48d51ca #1 2024-08-15 23:08:38 ~32 min windows/x86_64 💿exe
Commit #️⃣ Finished (UTC) Duration Platform Result
d4342c4 #2 2024-08-16 05:08:13 ~4 min macos/aarch64 📄log
✔️ d4342c4 #2 2024-08-16 05:10:21 ~6 min tests/nim 📄log
✔️ d4342c4 #2 2024-08-16 05:11:28 ~7 min macos/x86_64 🍎dmg
✔️ d4342c4 #2 2024-08-16 05:16:00 ~12 min linux-nix/x86_64 📦tgz
✔️ d4342c4 #2 2024-08-16 05:16:30 ~12 min tests/ui 📄log
✔️ d4342c4 #2 2024-08-16 05:19:06 ~15 min linux/x86_64 📦tgz
✔️ d4342c4 #2 2024-08-16 05:34:02 ~30 min windows/x86_64 💿exe
✔️ b9c224e #3 2024-08-22 18:38:23 ~5 min macos/aarch64 🍎dmg
✔️ b9c224e #3 2024-08-22 18:39:47 ~6 min tests/nim 📄log
✔️ b9c224e #3 2024-08-22 18:42:55 ~9 min macos/x86_64 🍎dmg
✔️ b9c224e #3 2024-08-22 18:44:26 ~11 min tests/ui 📄log
✔️ b9c224e #3 2024-08-22 18:46:57 ~13 min linux-nix/x86_64 📦tgz
✔️ b9c224e #3 2024-08-22 18:48:33 ~15 min linux/x86_64 📦tgz
✔️ b9c224e #3 2024-08-22 19:04:25 ~31 min windows/x86_64 💿exe

@friofry friofry force-pushed the ab/issue-15855-missing-images branch from 48d51ca to d4342c4 Compare August 16, 2024 05:03
Copy link
Member

@caybro caybro left a comment

Choose a reason for hiding this comment

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

LGTM (as a workaround)

But I can't really see the minted token at the end of the video (in CollectiblesView)?

@@ -983,4 +983,49 @@ QtObject {
// #15331
return true
}

function toBase64(buffer) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm.. I wonder how come, this should be identical

Copy link
Member

@jrainville jrainville left a comment

Choose a reason for hiding this comment

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

Seems like a fine workaround for now, but the video seems to show that the image didn't work in the wallet?

@friofry
Copy link
Contributor Author

friofry commented Aug 21, 2024

Seems like a fine workaround for now, but the video seems to show that the image didn't work in the wallet?

The minted token does not appear in the wallet until you restart the app. I've added a bug for this (#16173)

mint_image.mp4

Other tokens have a blank image because they were created before the fix

@jrainville
Copy link
Member

@friofry all good. Can you rebase on latest master. It might fix the build issue and also to make sure it works fine on latest still.

@friofry friofry force-pushed the ab/issue-15855-missing-images branch from d4342c4 to b9c224e Compare August 22, 2024 18:32
@friofry friofry merged commit d3131c2 into master Aug 23, 2024
9 checks passed
@friofry friofry deleted the ab/issue-15855-missing-images branch August 23, 2024 04:03
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.

Missing community owner token image
4 participants