Skip to content
This repository has been archived by the owner on Jan 24, 2022. It is now read-only.

Removed dependency names can no longer be reused. #217

Merged
merged 2 commits into from
Oct 10, 2018
Merged

Removed dependency names can no longer be reused. #217

merged 2 commits into from
Oct 10, 2018

Conversation

nventuro
Copy link
Contributor

Fixes #202.

I considered leaving a sentinel value to signal a name had been taken, but both owner and address need to be obviously zeroed out, and stake can be 0 (if minimalStake is 0), so I opted for adding an extra field. This should have a minimal impact on gas costs, since used is packed alongside owner, emitting a single SSTORE/SLOAD opcode when accessing (this was verified experimentally).

I'm not 100% sure on the name (maybe nameTaken?), but this is an internal detail and not part of the API.

@facuspagnuolo
Copy link
Contributor

An alternative could be to have a separate mapping to tell whether a given dependency name was used or not: mapping (string => bool) dependencyUsed
Then we can full delete the Dependency struct and check for that mapping when registering new ones. What do you think?

@nventuro
Copy link
Contributor Author

The only benefit of splitting them that way would be (arguably) cleaner code, in that you'd later be able to modify Dependency and not have to worry about keeping remove up to date, but it would make both create and remove more expensive (4 stores instead of 3).

The cost increase is not that large, and those functions shouldn't be used that often anyway, so it might be a reasonable trade-off.

@facuspagnuolo facuspagnuolo self-assigned this Oct 10, 2018
Copy link
Contributor

@facuspagnuolo facuspagnuolo left a comment

Choose a reason for hiding this comment

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

LGTM!

@facuspagnuolo facuspagnuolo added the status:ready-to-merge Order mergify to merge label Oct 10, 2018
@mergify mergify bot merged commit fe6dd9a into OpenZeppelin:master Oct 10, 2018
@nventuro nventuro deleted the vouching-name-reuse-fix branch October 10, 2018 17:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status:ready-to-merge Order mergify to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants