-
Notifications
You must be signed in to change notification settings - Fork 871
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
Tokens that fail to redeem due to network errors should be added to a queue and retried #2072
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
}; | ||
|
||
ConfirmationType() = default; | ||
constexpr ConfirmationType(const Value value) : value_(value) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lint error here, Single-parameter constructors should be marked explicit. [runtime/explicit] [5]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as we discussed in slack, I'm fine with NOLINT and a comment stating that we allow implicit conversion from enum value to this wrapper class, similar as https://cs.chromium.org/chromium/src/v8/src/wasm/value-type.h?rcl=88a0c6d607cf023a5776193fe1786be0ca9c0767&l=64-65.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 added NOLINT and comment as above, thanks
}; | ||
|
||
ConfirmationType() = default; | ||
constexpr ConfirmationType(const Value value) : value_(value) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DITTO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 added NOLINT and comment as above, thanks
a82f845
to
4f25fb3
Compare
… queue and retried
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fixes brave/brave-browser#3662
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security
) onnpm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Confirm failed requests are added to the
Defaults/rewards_service/confirmations.json
underconfirmations
/failed_confirmations
and confirmation redemption is retried every 5 minutes +/- jitter until successful. Items that are retried and fail should move to the back of the queue.Reviewer Checklist: