fix!: profile query returns nil if profile does not exist #971
Conversation
github-actions
bot
added
the
x/profiles
| @@ -47,7 +47,7 @@ func (k Keeper) Profile(ctx context.Context, request *types.QueryProfileRequest) | |||
| } | |||
|
|
|||
| if !found { | |||
| return &types.QueryProfileResponse{Profile: nil}, nil | |||
| return nil, status.Errorf(codes.NotFound, "profile for dtag/address %s not found", dTagOrAddress) | |||
There was a problem hiding this comment.
This is something I've thought about for a while, and honestly I don't remember why we chose to return nil instead of an error. However, I would like to take this chance to start a conversation about this. Particularly, I'm interested in knowing your opinion about the pros and cons that returning an error instead of nil might have.
I would like to ask you guys (@dadamu, @bragaz, @manu0466) considering your experience in smart contract development, as well as @ale-mazz considering his experience in client-side development. Do you think that a missing profile should return an error or null, and why?
There was a problem hiding this comment.
This issue is found during devloping the POAP-manager contract.
- If it returns nil, the contract side need to handle the
Optionunwrap process like:
let profile_res = ProfilesQuerier::new(deps.querier.deref()).query_profile(user)?;
match profile_res.profile {
Some(profile) => Ok(operate(profile)),
None => Err(some error)
}- If it returns an error, then the process can be simplified to be as follow since the
non found profilecase has been already handled by error:
let profile_res = ProfilesQuerier::new(deps.querier.deref()).query_profile(user)?;
operate(profile_res.profile)Besides, frontend also need a check if the profile exists or not if it returns nil.
As a result, I would use returning error instead if the specific item does not exist.
There was a problem hiding this comment.
In my opinion, it would be more correct to return an error, so the frontend developer would have an advantage in relaunching the error in the UI or handling it in the way they prefer. Returning null might lead one to think of the presence of a possible backend error or otherwise something incorrect. I always consider error handling rather than returning null, undefined, etc. values to be superior.
There was a problem hiding this comment.
Ok thanks for the contributions. I think the best thing for clients then is to simply return nil so I will accept this PR as soon as @dadamu adds the proper changeset entry.
Codecov Report
@@ Coverage Diff @@
## master #971 +/- ##
==========================================
- Coverage 80.54% 80.50% -0.04%
==========================================
Files 172 172
Lines 15095 15223 +128
==========================================
+ Hits 12158 12256 +98
- Misses 2415 2440 +25
- Partials 522 527 +5
Continue to review full report at Codecov.
|
RiccardoM
changed the title
|
@RiccardoM Added. |
.changeset/entries/e8fdc65b9f41e85409395d7e7b317f59f6d7be1ff3f8a92ebb30dafeb3254d4e.yaml
Outdated
Show resolved
Hide resolved
…8a92ebb30dafeb3254d4e.yaml
Description
Currently, querying a non-existing profile returns
nilinstead of error withprofile not found for address/dtag.This PR fixes the response of
Profilebehavior mentioned above.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change