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

Bugfix: NIP-05 strikethrough in bad networks #1210

Merged
merged 3 commits into from
Jun 4, 2024

Conversation

martindsq
Copy link
Member

@martindsq martindsq commented Jun 3, 2024

Issues covered

#1161

Description

While we don't show the NIP-05 with a strikethrough when we are fetching the server response, we do show it if the request fails. In order to fix this specific issue, I'm catching the "cancel" reason. This happens when we cancel the request programmatically, but also frequently happens when the user has a very bad network (I'm not sure why, we don't cancel the request programmatically, I want to find out).

Also, relevant discussion here https://planetary-app.slack.com/archives/CERN44F17/p1717190530026669?thread_ts=1717185627.726309&cid=CERN44F17

How to test

  1. Open iOS Settings
  2. Go to the Developer section
  3. Enable the network conditioner (not sure the exact words iOS when used in English) and select the Very Bad Network option
  4. Go to Nos
  5. Open the Discover screen because it displays many users with a NIP-05
  6. In the production build, these NIPs are frequently strikethrough-ed, in this branch it should not.

Screenshots/Video

It doesn't make sense to post a screenshot for the "after" state. But I can post a screenshot of how the bug looks when using the app with a bad network:

IMG_8255

}
self.verifiedNip05Identifier = isVerified
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the animation because for the normal use case (a valid NIP-05), the animation causes an annoying blink. See relevant comment here https://planetary-app.slack.com/archives/CM4EPK324/p1717187450297749

I tried to add an if that executes that just when isVerified is different from verifiedNip05Identifier but it didn't work right away and decided not to continue with that line just because I think the animation is not important in this case and we can just remove it for all cases. If it were important, I think we should re-try the if line, eventually we will find out the solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me.

@martindsq martindsq changed the title 1161/bugfix nip05 strikethrough Bugfix: NIP-05 strikethrough in bad networks Jun 3, 2024
Copy link
Contributor

@joshuatbrown joshuatbrown left a comment

Choose a reason for hiding this comment

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

Just a couple minor comments. I'd prefer to have some small changes, but they're not blockers.

}
self.verifiedNip05Identifier = isVerified
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me.

Nos/Views/NIP05View.swift Show resolved Hide resolved
Comment on lines 47 to 49
// Cancelled requests are common in bad networks. We
// should keep things as they were to avoid reproducing
// https://github.com/planetary-social/nos/issues/1161
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate the comment here to explain what's happening. I think I'd go a little further -- in order to fully understand this comment, you need to open the link. What if we include a brief description of the issue right here so the reader doesn't have to follow the link? We can still keep the link so they can get more detail.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I agree with you. Let me correct it.

@rabble
Copy link
Contributor

rabble commented Jun 3, 2024

This PR is fine but also kind of misses the point. We should be caching the status of nip-05 verification and using that. The most common thing to happen is the user sets a nip-05 verification and then the server is flaky. In that case we should continue to show it as verified while somehow some slow background process might check to update.

Failure to connect should not reset from verified to not, it’d need to be something more like that service IS online but no longer serving the valid data fro this user.

@martindsq
Copy link
Member Author

This PR is fine but also kind of misses the point. We should be caching the status of nip-05 verification and using that. The most common thing to happen is the user sets a nip-05 verification and then the server is flaky. In that case we should continue to show it as verified while somehow some slow background process might check to update.

Failure to connect should not reset from verified to not, it’d need to be something more like that service IS online but no longer serving the valid data fro this user.

I totally agree with you @rabble . As I explained in this slack comment yesterday, we should cache the NIP-05 validation, we are doing it all the time and that's too much. I'll open a ticket so @setch-l is aware of that and can prioritize that work accordingly.

@martindsq
Copy link
Member Author

@rabble @setch-l I filed #1215

@martindsq martindsq requested a review from joshuatbrown June 4, 2024 13:54
Copy link
Contributor

@joshuatbrown joshuatbrown left a comment

Choose a reason for hiding this comment

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

Great work!

@martindsq martindsq added this pull request to the merge queue Jun 4, 2024
Merged via the queue into main with commit 9bc8475 Jun 4, 2024
5 checks passed
@martindsq martindsq deleted the 1161/bugfix_nip05_strikethrough branch June 4, 2024 18:54
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.

3 participants