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 nonce validation error #152

Merged
merged 1 commit into from
Feb 8, 2024
Merged

Fix nonce validation error #152

merged 1 commit into from
Feb 8, 2024

Conversation

barebind
Copy link
Contributor

@barebind barebind commented Feb 8, 2024

Resolves #149

Fix nonce validation error by mimicking the original solidity code.

@0x4007 0x4007 requested a review from rndquu February 8, 2024 05:30
Copy link

ubiquibot bot commented Feb 8, 2024

@barebind barebind marked this pull request as ready for review February 8, 2024 07:10
@barebind barebind requested a review from 0x4007 as a code owner February 8, 2024 07:10
@rndquu
Copy link
Member

rndquu commented Feb 8, 2024

@barebind Did you try that the code works as expected?

@barebind
Copy link
Contributor Author

barebind commented Feb 8, 2024

@rndquu yes I've tested multiple cases. used/non-used nonces, connected wallets etc.

one thing though, if you try to test, the generate-permit2-url.ts script doesn't work in the development branch. I've fixed that too but didn't commit because it's already committed in another PR

@rndquu
Copy link
Member

rndquu commented Feb 8, 2024

yes I've tested multiple cases. used/non-used nonces, connected wallets etc.

I'm asking because (as far as I remember) this is the correct implementation of checking whether nonce is valid. Current PR introduces the xor operation which doesn't exist here.

one thing though, if you try to test, the generate-permit2-url.ts script doesn't work in the development branch

It's kind of obsolete, could be removed but it's a useful reference.

I've fixed that too but didn't commit because it's already committed in another #151

Overall pls follow the "1 PR solves exactly 1 issue rule", otherwise it's really hard to review PRs

@barebind
Copy link
Contributor Author

barebind commented Feb 8, 2024

I've implemented/mimicked from the solidity code which has exactly same functionality actually. Current implementation (also the one that you referenced) never worked as expected for me on my local.

Overall pls follow the "1 PR solves exactly 1 issue rule", otherwise it's really hard to review PRs

Sure.

Copy link
Contributor

@gitcoindev gitcoindev left a comment

Choose a reason for hiding this comment

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

All right this is in line with my discovery. ubiquity/ubiquibot-kernel#5 (comment) , the change looks good and I am also approving from my side.

@gitcoindev gitcoindev merged commit 659ec67 into ubiquity:development Feb 8, 2024
1 check passed
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.

Unexpected Error Message + Can't Claim
3 participants