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.

Sign up for GitHub

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

Jump to bottom

identity: small improvements #458

Merged
merged 2 commits into from
Nov 29, 2023
Merged

identity: small improvements #458

merged 2 commits into from
Nov 29, 2023

Conversation

bnewbold
Copy link
Collaborator

I'm not sure why the error wasn't getting returned; might have been a typo? Probably worth re-reading that function again in review. I hit a nil de-reference pointer in automod caused by that function returning nil, nil.

ericvolp12
ericvolp12 approved these changes Nov 29, 2023
View reviewed changes
Copy link
Collaborator

@ericvolp12 ericvolp12 left a comment

Choose a reason for hiding this comment

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

LGTM

@ericvolp12
Copy link
Collaborator

I'm not sure why the error wasn't getting returned; might have been a typo? Probably worth re-reading that function again in review. I hit a nil de-reference pointer in automod caused by that function returning nil, nil.

I don't really get how that function can ever return nil, nil though, entry is always set as something and we always return a pointer to a real entry object.

@ericvolp12
Copy link
Collaborator

Aren't we already returning the error on the entry object as well? I'm fine returning it twice but just something to think about.

@bnewbold
Copy link
Collaborator Author

Ah, sorry, to clarify it was the call of updateDID() in LookupDID (https://github.com/bluesky-social/indigo/pull/458/files#diff-b91976d7b63c04bc9f338df9dba703c4441b915b0535ea598821efa1350d25f9R216-R219) which was unpacking the Identity as doc (nil) and returned err (also nil) and return doc, err.

We could have LookupDID unpack the internal Err from the entry doc, but that feels a bit convoluted to me. I think of the Err in the entry doc as only being used for cache persistence.

@bnewbold bnewbold merged commit 81f67da into main Nov 29, 2023
7 checks passed
@bnewbold bnewbold deleted the bnewbold/identity-tweaks branch November 29, 2023 22:59
@bnewbold bnewbold mentioned this pull request Dec 15, 2023
Merged
bnewbold added a commit that referenced this pull request Dec 15, 2023
@bnewbold
identity: refactor cache directory to prevent nil/nil condition (#479)
a0cf12c 
This is closely related to
#458, which tried to
resolve an issue with DIDs. I was seeing very confusing behavior with
handles; was able to confirm `"", nil` getting returned internally on
real-world lookups.

I took a more aggressive approach and changed the return signatures of
both update helper methods to just return an "Entry", not as a pointer,
and no err. This makes the code flow much simpler.

I tacked on a special control-flow error message as defensive
programming after getting bit twice on this (darn self!). Probably isn't
necessary and smells a bit but 🤷.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericvolp12 ericvolp12 ericvolp12 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bnewbold @ericvolp12