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

fix(authorization): revert already set error #652

Merged
merged 4 commits into from
Dec 20, 2023

Conversation

Rubilmax
Copy link
Collaborator

@Rubilmax Rubilmax marked this pull request as ready for review December 18, 2023 17:43
Copy link
Contributor

@QGarchery QGarchery left a comment

Choose a reason for hiding this comment

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

Should also remove the corresponding test, but otherwise LGTM

Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

we don't delete it for setAuthorization? I feel this is useless as well and a bit weird to have 2 different requirements for the same functionality

@Rubilmax
Copy link
Collaborator Author

we don't delete it for setAuthorization? I feel this is useless as well and a bit weird to have 2 different requirements for the same functionality

This argument can also be used to defend the introduction of such requirement: why would we require not already set fee but not in setAuthorization* functions?

In the end, I am convinced we were inconsistent, though I am not convinced it is a big deal...

@MerlinEgalite
Copy link
Contributor

But now we're being weirdly inconsistent it's even more disturbing to me

@Rubilmax
Copy link
Collaborator Author

But now we're being weirdly inconsistent it's even more disturbing to me

Mb I wasn't clear: I was arguing we shouldn't revert the change at all!

@Rubilmax
Copy link
Collaborator Author

Another rationale behind this PR would be to simply revert the changes to the old way, even if it can be considered inconsistent, because it still works and there's no big deal

@QGarchery
Copy link
Contributor

QGarchery commented Dec 20, 2023

why would we require not already set fee but not in setAuthorization* functions?

For setAuthorizationWithSig it's because the function is inherently more complicated and signatures are asynchronous by nature. I don't feel like just because it looks disturbing it means that it is incorrect.

You can also look at it another way: we are never in the "already set" case for setAuthorizationWithSig because it's at least changing the nonce. If we really want to be consistent, we could change INVALID_NONCE into ALREADY_SET (but I don't feel like it's necessary)

@MathisGD MathisGD self-assigned this Dec 20, 2023
Copy link
Collaborator Author

@Rubilmax Rubilmax left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

Let's add a comment no?

@Rubilmax Rubilmax mentioned this pull request Dec 20, 2023
@Rubilmax
Copy link
Collaborator Author

Let's add a comment no?

Something like:
/// Do not check whether authorization is already set because the nonce increment is a desired side-effect.

@MerlinEgalite
Copy link
Contributor

Let's add a comment no?

Something like: /// Do not check whether authorization is already set because the nonce increment is a desired side-effect.

LGTM

@MerlinEgalite MerlinEgalite merged commit 24b4d02 into post-cantina Dec 20, 2023
15 checks passed
@MerlinEgalite MerlinEgalite deleted the fix/revert-already-set branch December 20, 2023 11:17
@MerlinEgalite MerlinEgalite mentioned this pull request Dec 20, 2023
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.

6 participants