-
Notifications
You must be signed in to change notification settings - Fork 12.3k
feat: add NonceTracker contract #1022
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
Conversation
|
I want to refactor this to use the AutoIncrementing library. Depends on #1023 |
|
Actually, after trying to refactor, I'm not convinced it benefits from using AutoIncrementing at all. @frangio I see you gave a 👍 ; any thoughts? |
|
|
||
| /** | ||
| * @dev call this function when accepting a nonce | ||
| * @dev throws if nonce is not strictly greater than previous nonce |
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.
Technically, it reverts 😛
| import "../access/NonceTracker.sol"; | ||
|
|
||
|
|
||
| contract NonceTrackerImpl is NonceTracker { |
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.
Shouldn't this be called NonceTrackerMock, for consistency with the rest of the directory?
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.
the pattern I've been using for mock/impl is mock == use a fully-functional implementation, but make it more accessible for testing. implementation == implement some other thing in a way that a real contract would.
but we haven't really followed that at all afaik, so ¯\_(ツ)_/¯
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.
Haha I see. I'll take a look at making them consistent in the near future then.
| contract NonceTracker { | ||
| mapping(address => uint256) private nonces; | ||
|
|
||
| modifier withAccess(address _address, uint256 _nonce) |
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.
What's the _nonce value supposed to be here? I'm not really sure what the intended usage of this modifier is (there are no docs for it btw).
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.
I will add docs :)
But it's intended to "register" your usage of some resource, given a specific nonce. For example, using Bouncer, you would pass a custom nonce to avoid replay attacks and invalidate previous signatures. NonceTracker keeps track of the latest nonces and enforces that they only increase
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.
Oh I get it now. A bit convoluted though, isn't it? Since you're free to pass any nonce you wish, I don't really see a use for this other than the Bouncer. It's also wildly different from withMaxAccess (and incompatible, using one in a contract will break the other), to the point where I'm not sure if it shouldn't be its own thing (a Bouncer extension to prevent replaying txs).
| @@ -0,0 +1,67 @@ | |||
|
|
|||
| import assertRevert from '../helpers/assertRevert'; | |||
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.
We're using require as of #1074 :)
|
|
||
| contract('NonceTracker', ([_, owner, anyone, another]) => { | ||
| context('canDoThisOnce', function () { | ||
| before(async function () { |
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.
I see what you're doing here with the test cases following a sequence to prevent code duplication, but I'd rather them be a bit longer and independent - fragile test suites are always a PITA.
|
I take it you intended to use |
|
Mostly because AutoIncrementing doesn't (shouldn't?) support manually setting the previous id. It's designed for incrementing ids of things, not tracking nonces. So to support nonce tracking it'd have to have a setter for manually setting the |
|
Agreed, but it'd work if we restricted this to us the |
|
I think it's necessary to allow arbitrary nonce submissions, since incrementing won't always be done on-chain (like when using the bouncer pattern; I want to be able to invalidate previous nonces by signing a new nonce and submitting that on-chain). |
|
closing as part of #1272 |
🚀 Description