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

Add nonce to ?cond command image fetch #323

Closed
wants to merge 4 commits into from

Conversation

IvanGirderboot
Copy link

@IvanGirderboot IvanGirderboot commented Dec 14, 2020

Description

Fixes #322

Type of change

  • Bug fix

How has this been tested?

running fork in production

Checklist

  • Issue exists for PR
  • Code reviewed by the author
  • Code documented (comments or other documentation)
  • Changes tested
  • All tests pass (see DEVELOPING.md, if it exists)
  • CHANGELOG.md updated if needed
  • Informative commit messages
  • Descriptive PR title

@IvanGirderboot IvanGirderboot marked this pull request as ready for review December 14, 2020 02:24
@0x5c
Copy link
Member

0x5c commented Jan 18, 2021

Hello
Sorry for the long delay, life got in the way of doing much work on the bot.

First of all, thanks for submitting a PR, we really appreciate contributions to our projects 🙂.
Unfortunately, there's a couple of problems with this PR that mean we can't merge it.

  1. The fix you first copied turned out to not actually work as intended. As seen in Set date parameter for greyline instead of nonce #318, it caused wacky caching issues for anyone looking at the message a couple of hours after it was posted.
  2. Your new fix imports datetime to get the current minute, despite discord IDs being timestamps with additional data that guarantees uniqueness (the first version of the fix would extract the time from the ID of the received message, ctx.message.id). At one minute of delay, this simply is bruteforcing of the cache, and using a guaranteed unique value (like the ID) would make more sense.
  3. That new fix still does not solve the bug observed in Set date parameter for greyline instead of nonce #318.
  4. We have not yet found a fix that actually gets rid of the issue (except for ?greyline since the server accepts a date parameter and return an image according to it, PR Fixed ?greyline caching (set date url param) #335).
  5. The issue affects other commands.

Normally I'd just post a review with the elements to change, but since this will require an investigation on our part before a fix can be implemented, I am closing this PR.

@0x5c 0x5c closed this Jan 18, 2021
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.

All images are cached by Discord
2 participants