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

feat: r/profile dapp #1983

Merged
merged 46 commits into from
Jul 14, 2024
Merged

feat: r/profile dapp #1983

merged 46 commits into from
Jul 14, 2024

Conversation

kazai777
Copy link
Contributor

@kazai777 kazai777 commented Apr 25, 2024

Following this PR concerning the creation of a realm profile I created this realm which allows the creation of profile as well as the associated functions to display the information of a profile with an address or a username. I have some questions concerning this realm:

  • Currently, if a user modifies his username, his old username is not freed and is therefore no longer available, even if it is no longer in use. Should I free the old username when the user has changed username?
  • To make it possible to search by username and address, I've created a second avl tree containing both username and address, so that I can find the profile indexed by its address by searching for it by its username. This is the most efficient solution I've found. I'd like to get some feedback on this and know if I should do things differently so that searching by username is more optimized.
  • Do you have any other suggestions for completing the profile fields, or other interesting features to add?

Thanks in advance for your feedback

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

Closes #181

@kazai777 kazai777 requested review from a team as code owners April 25, 2024 15:04
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Apr 25, 2024
@leohhhn leohhhn self-requested a review April 29, 2024 07:46
Copy link
Contributor

@leohhhn leohhhn left a comment

Choose a reason for hiding this comment

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

Nice work - I left some comments that should be addressed before merging. Please take a look 🙏

examples/gno.land/r/demo/profile/profile.gno Outdated Show resolved Hide resolved
examples/gno.land/r/demo/profile/profile.gno Outdated Show resolved Hide resolved
examples/gno.land/r/demo/profile/profile.gno Outdated Show resolved Hide resolved
examples/gno.land/r/demo/profile/profile.gno Outdated Show resolved Hide resolved
examples/gno.land/r/demo/profile/profile.gno Outdated Show resolved Hide resolved
examples/gno.land/r/demo/profile/profile.gno Outdated Show resolved Hide resolved
examples/gno.land/r/demo/profile/profile.gno Outdated Show resolved Hide resolved
examples/gno.land/r/demo/profile/profile.gno Outdated Show resolved Hide resolved
examples/gno.land/r/demo/profile/profile.gno Outdated Show resolved Hide resolved
examples/gno.land/r/demo/profile/profile.gno Outdated Show resolved Hide resolved
examples/gno.land/r/demo/profile/profile.gno Outdated Show resolved Hide resolved
examples/gno.land/r/demo/profile/profile.gno Outdated Show resolved Hide resolved
examples/gno.land/r/demo/profile/profile.gno Outdated Show resolved Hide resolved
Copy link

codecov bot commented May 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.18%. Comparing base (977a3f4) to head (38b5439).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1983      +/-   ##
==========================================
- Coverage   55.01%   54.18%   -0.83%     
==========================================
  Files         481      520      +39     
  Lines       67432    73038    +5606     
==========================================
+ Hits        37097    39576    +2479     
- Misses      27318    30245    +2927     
- Partials     3017     3217     +200     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@moul
Copy link
Member

moul commented Jul 4, 2024

It looks good. I believe I made the last batch of reviews. Please update and notify me so I can make the final round of reviews and merge.

Copy link
Contributor

@leohhhn leohhhn left a comment

Choose a reason for hiding this comment

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

Apologies for the late review 🙏

I love this; I even didn't know that gnoweb can be used this way. Thank you for this example. Code feels clean as well 👍

@kazai777 kazai777 requested a review from moul July 13, 2024 10:39
@moul moul merged commit 5c57f21 into gnolang:master Jul 14, 2024
9 checks passed
@jefft0
Copy link
Contributor

jefft0 commented Jul 15, 2024

Hello @kazai777 . Congratulations on merging this PR. We want to use it in dSocial to store the avatar image. Should this be standardized in some way? For example, the string should a base64-encoded png image. Or should it be an HTML img tag? I know you want to be flexible, but I'm curious what is your intended use.

@kazai777
Copy link
Contributor Author

Congratulations on merging this PR. We want to use it in dSocial to store the avatar image. Should this be standardized in some way? For example, the string should a base64-encoded png image. Or should it be an HTML img tag? I know you want to be flexible, but I'm curious what is your intended use.

Hi @jefft0. I'm glad you want to use r/profile for dSocial :). I think it's best to standardize the storage of the avatar image by encoding it in base64. This ensures that the image is always available without depending on external sources, and allows content validation and control before storage. By standardizing the storage of avatar images in base64-encoded strings, this guarantees greater data security and control.

@jefft0
Copy link
Contributor

jefft0 commented Jul 15, 2024

Congratulations on merging this PR. We want to use it in dSocial to store the avatar image. Should this be standardized in some way? For example, the string should a base64-encoded png image. Or should it be an HTML img tag? I know you want to be flexible, but I'm curious what is your intended use.

Hi @jefft0. I'm glad you want to use r/profile for dSocial :). I think it's best to standardize the storage of the avatar image by encoding it in base64. This ensures that the image is always available without depending on external sources, and allows content validation and control before storage. By standardizing the storage of avatar images in base64-encoded strings, this guarantees greater data security and control.

Yes, we will base-64 encode the image. When retrieving the image, how to know if it is a png or jpg image?

@kazai777
Copy link
Contributor Author

Congratulations on merging this PR. We want to use it in dSocial to store the avatar image. Should this be standardized in some way? For example, the string should a base64-encoded png image. Or should it be an HTML img tag? I know you want to be flexible, but I'm curious what is your intended use.

Hi @jefft0. I'm glad you want to use r/profile for dSocial :). I think it's best to standardize the storage of the avatar image by encoding it in base64. This ensures that the image is always available without depending on external sources, and allows content validation and control before storage. By standardizing the storage of avatar images in base64-encoded strings, this guarantees greater data security and control.

Yes, we will base-64 encode the image. When retrieving the image, how to know if it is a png or jpg image?

It might be a good idea to add a prefix to the encoded string. Something like data:img/png or data:img/jpg. This would make it easy to check the type of image when retrieving it.

@jefft0
Copy link
Contributor

jefft0 commented Jul 15, 2024

Congratulations on merging this PR. We want to use it in dSocial to store the avatar image. Should this be standardized in some way? For example, the string should a base64-encoded png image. Or should it be an HTML img tag? I know you want to be flexible, but I'm curious what is your intended use.

Hi @jefft0. I'm glad you want to use r/profile for dSocial :). I think it's best to standardize the storage of the avatar image by encoding it in base64. This ensures that the image is always available without depending on external sources, and allows content validation and control before storage. By standardizing the storage of avatar images in base64-encoded strings, this guarantees greater data security and control.

Yes, we will base-64 encode the image. When retrieving the image, how to know if it is a png or jpg image?

It might be a good idea to add a prefix to the encoded string. Something like data:img/png or data:img/jpg. This would make it easy to check the type of image when retrieving it.

This is telling me, to invent something. So, I understand the answer as "No, there is no specific recommended way to store the avatar image."

@kazai777
Copy link
Contributor Author

Congratulations on merging this PR. We want to use it in dSocial to store the avatar image. Should this be standardized in some way? For example, the string should a base64-encoded png image. Or should it be an HTML img tag? I know you want to be flexible, but I'm curious what is your intended use.

Hi @jefft0. I'm glad you want to use r/profile for dSocial :). I think it's best to standardize the storage of the avatar image by encoding it in base64. This ensures that the image is always available without depending on external sources, and allows content validation and control before storage. By standardizing the storage of avatar images in base64-encoded strings, this guarantees greater data security and control.

Yes, we will base-64 encode the image. When retrieving the image, how to know if it is a png or jpg image?

It might be a good idea to add a prefix to the encoded string. Something like data:img/png or data:img/jpg. This would make it easy to check the type of image when retrieving it.

This is telling me, to invent something. So, I understand the answer as "No, there is no specific recommended way to store the avatar image."

There's no standard at the moment, but encoding the image in base64 is just what I would have done personaly. But your remark is interesting, because I could add something to add a prefix according to the image format and retrieve it easily, without you needing to create something new.

@jefft0
Copy link
Contributor

jefft0 commented Jul 15, 2024

It might be a good idea to add a prefix to the encoded string. Something like data:img/png or data:img/jpg. This would make it easy to check the type of image when retrieving it.

This is telling me, to invent something. So, I understand the answer as "No, there is no specific recommended way to store the avatar image."

There's no standard at the moment, but encoding the image in base64 is just what I would have done personaly. But your remark is interesting, because I could add something to add a prefix according to the image format and retrieve it easily, without you needing to create something new.

The great thing about r/demo/profile is that many Gno.land apps can use it. Maybe a user first adds their avatar from a different app. Now the user joins dSocial and dSocial needs to display their avatar. This nice feature of a common store for user profile info is why I'm asking about standardization. I'll be happy with whatever solution you choose, but I think it should be documented in the code. Either combine the mime type with the base64 encoding, or add a new field AvatarMimeType which is something like "image/png".

@jefft0
Copy link
Contributor

jefft0 commented Jul 16, 2024

There's no standard at the moment, but encoding the image in base64 is just what I would have done personaly. But your remark is interesting, because I could add something to add a prefix according to the image format and retrieve it easily, without you needing to create something new.

The great thing about r/demo/profile is that many Gno.land apps can use it. Maybe a user first adds their avatar from a different app. Now the user joins dSocial and dSocial needs to display their avatar. This nice feature of a common store for user profile info is why I'm asking about standardization. I'll be happy with whatever solution you choose, but I think it should be documented in the code. Either combine the mime type with the base64 encoding, or add a new field AvatarMimeType which is something like "image/png".

Good. So we agree that there should be a suggested standard for the Avatar image format. What is the next step? Maybe one of these options:

  1. jefft0 makes a PR to add a field for AvatarMimeType. (This is preferred by the Berty team since it is more clear and doesn't require parsing the Avatar field.)
  2. jefft0 makes a PR to add a comment to the Avatar field to suggest that the format should be "<base64-image>:<mime-type>". (This is a fallback if you don't like adding a new field as in option 1.)
  3. Open a new issue to to let @kazai777 and the Gno devs think about it more and fix it later.

@kazai777
Copy link
Contributor Author

There's no standard at the moment, but encoding the image in base64 is just what I would have done personaly. But your remark is interesting, because I could add something to add a prefix according to the image format and retrieve it easily, without you needing to create something new.

The great thing about r/demo/profile is that many Gno.land apps can use it. Maybe a user first adds their avatar from a different app. Now the user joins dSocial and dSocial needs to display their avatar. This nice feature of a common store for user profile info is why I'm asking about standardization. I'll be happy with whatever solution you choose, but I think it should be documented in the code. Either combine the mime type with the base64 encoding, or add a new field AvatarMimeType which is something like "image/png".

Good. So we agree that there should be a suggested standard for the Avatar image format. What is the next step? Maybe one of these options:

  1. jefft0 makes a PR to add a field for AvatarMimeType. (This is preferred by the Berty team since it is more clear and doesn't require parsing the Avatar field.)
  2. jefft0 makes a PR to add a comment to the Avatar field to suggest that the format should be "<base64-image>:<mime-type>". (This is a fallback if you don't like adding a new field as in option 1.)
  3. Open a new issue to to let @kazai777 and the Gno devs think about it more and fix it later.

I think the third option is a good one. This will allow other developers to give their opinion on the subject and depending on the feedback, I'll fix it with the best way for everyone.

@jefft0
Copy link
Contributor

jefft0 commented Jul 17, 2024

Good. So we agree that there should be a suggested standard for the Avatar image format. What is the next step? Maybe one of these options:

  1. jefft0 makes a PR to add a field for AvatarMimeType. (This is preferred by the Berty team since it is more clear and doesn't require parsing the Avatar field.)
  2. jefft0 makes a PR to add a comment to the Avatar field to suggest that the format should be "<base64-image>:<mime-type>". (This is a fallback if you don't like adding a new field as in option 1.)
  3. Open a new issue to to let @kazai777 and the Gno devs think about it more and fix it later.

I think the third option is a good one. This will allow other developers to give their opinion on the subject and depending on the feedback, I'll fix it with the best way for everyone.

OK, thanks. I created the issue. #2598

gfanton pushed a commit to gfanton/gno that referenced this pull request Jul 23, 2024
Following this
[PR](gnolang#181 (comment))
concerning the creation of a realm `profile` I created this realm which
allows the creation of profile as well as the associated functions to
display the information of a profile with an address or a username. I
have some questions concerning this realm:

- Currently, if a user modifies his username, his old username is not
freed and is therefore no longer available, even if it is no longer in
use. Should I free the old username when the user has changed username?
- To make it possible to search by username and address, I've created a
second avl tree containing both username and address, so that I can find
the profile indexed by its address by searching for it by its username.
This is the most efficient solution I've found. I'd like to get some
feedback on this and know if I should do things differently so that
searching by username is more optimized.
- Do you have any other suggestions for completing the profile fields,
or other interesting features to add?

Thanks in advance for your feedback

<details><summary>Contributors' checklist...</summary>

- [ ] Added new tests, or not needed, or not feasible
- [ ] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [ ] Updated the official documentation or not needed
- [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [ ] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

Closes gnolang#181

---------

Co-authored-by: kazai <kazai@777>
Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>
Co-authored-by: Leon Hudak <33522493+leohhhn@users.noreply.github.com>
zivkovicmilos pushed a commit that referenced this pull request Sep 11, 2024
Related to this [PR](#1983) and
[Manfred's
idea](#1983 (comment)),
this PR aims at adding possibility to add arbitrary field to profile.

I keep the same fields list defined in the struct, they would be served
as "standard/base" fields and all other arbitrary fields are considered
as "extra".



<!-- please provide a detailed description of the changes made in this
pull request. -->

<details><summary>Contributors' checklist...</summary>

- [ ] Added new tests, or not needed, or not feasible
- [ ] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [ ] Updated the official documentation or not needed
- [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [ ] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Signed-off-by: Norman Meier <norman@samourai.coop>
Co-authored-by: Norman Meier <norman@samourai.coop>
Co-authored-by: n0izn0iz <n0izn0iz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: Done
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants