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

feat: add AutoIncrementing contract #1023

Merged
merged 7 commits into from
Jul 18, 2018

Conversation

shrugs
Copy link
Contributor

@shrugs shrugs commented Jun 17, 2018

/**
 * @title AutoIncrementing
 * @author Matt Condon (@shrugs)
 * @dev Provides an auto-incrementing uint256 id acquired by the `nextId` getter.
 * Use this for issuing ERC721Token Ids or keeping track of request ids, anything you want, really.
 * @notice Does not allow an Id of 0, which is popularly used to signify a null state in solidity.
 */

@shrugs shrugs mentioned this pull request Jun 17, 2018
4 tasks
@frangio
Copy link
Contributor

frangio commented Jun 27, 2018

I feel like this would be better as a library with a struct, so that a contract could have more than one auto-incrementing number. Structs don't have private fields though, so that would blow out the nice encapsulation, but I don't think it's too big a deal.

@shrugs
Copy link
Contributor Author

shrugs commented Jun 29, 2018

@frangio can you explain in more detail what you mean?

@frangio
Copy link
Contributor

frangio commented Jun 29, 2018

Yes, sorry for the terseness. 🙇‍♂️

What I wanted to say is that implementing this functionality by inheritance doesn't feel right to me. The main reason why I feel this way is that it only allows for one auto-incrementing counter per contract. Conceptually, "auto-incrementing counter" is a new datatype, with an operation "next", so I would expect to be able to create several instances of it.

Solidity allows the creation of new datatypes using libraries and structs. In this case it would be something like:

struct Counter {
    uint256 _prev;
}

function next(Counter counter) internal returns (uint256);

Used as:

Counter private ids;
...
    uint256 newId = ids.next();

This enables having multiple counters and doesn't add to the inheritance graph of a contract. On the other hand, whereas the contract implementation in this PR is nicely encapsulated by having the nextId_ be private, structs don't have private members, and users of the struct can modify its members arbitrarily.

I think Solidity should have private struct members, but that isn't there yet. (Is there even an issue?) In the meantime, I think I still prefer the struct implementation to the inheritance one.

Thoughts?

@frangio frangio self-requested a review July 10, 2018 03:25
@shrugs shrugs force-pushed the feat/autoincrementing branch 2 times, most recently from 3b5bb7f to b18cede Compare July 18, 2018 20:17
@shrugs
Copy link
Contributor Author

shrugs commented Jul 18, 2018

@frangio I've update the contract using that spec, and I like it better! Can you give it a last review?

@shrugs shrugs self-assigned this Jul 18, 2018
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Looks nice!

I'd prefer to keep the linting changes on unrelated files out of this PR.

*
* Include with `using AutoIncrementing for AutoIncrementing.Counter;`
* @notice Does not allow an Id of 0, which is popularly used to signify a null state in solidity.
* Does not protect from overflows, but if you have 2^256 ids, you have other problems.
Copy link
Contributor

Choose a reason for hiding this comment

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

LOL. It's actually physically impossible (as in not enough energy in the universe-impossible) to overflow a counter like this. This line is funny though, maybe we can just add in parenthesis that for reals it will never overflow so that people don't worry unnecessarily.

@shrugs
Copy link
Contributor Author

shrugs commented Jul 18, 2018

fixed!

@shrugs shrugs merged commit 3318b91 into OpenZeppelin:master Jul 18, 2018
@shrugs shrugs deleted the feat/autoincrementing branch July 18, 2018 23:38
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.

2 participants