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

Identifier uniqueness isn't enforced #253

Open
matthewlilley opened this issue Oct 15, 2021 · 2 comments
Open

Identifier uniqueness isn't enforced #253

matthewlilley opened this issue Oct 15, 2021 · 2 comments

Comments

@matthewlilley
Copy link
Member

matthewlilley commented Oct 15, 2021

Pool implementations require a poolIdentifier to satisfiy IPool.

/// @return A unique identifier for the pool type.
function poolIdentifier() external pure returns (bytes32);

However, uniqueness is not enforced, it probably makes sense to do this.

I did suggest an alternative too which might be worth thinking about, and could be quite useful for consumers, a pool indentifier + version e.g ID: ConstantProductPool, VERSION: 1 etc..., uniqueness of the combination would need to be enforced.

@z0r0z
Copy link
Contributor

z0r0z commented Oct 18, 2021

uniqueness shouldn't be hard to enforce by adding a check in MasterDeployer and the addToWhitelist function (& storing the identifier for each pool (factory)). So, no extra costs to Trident users. Though, arguably, we likely can avoid these errors in careful mgmt of this role.

@matthewlilley
Copy link
Member Author

uniqueness shouldn't be hard to enforce by adding a check in MasterDeployer and the addToWhitelist function (& storing the identifier for each pool (factory)). So, no extra costs to Trident users. Though, arguably, we likely can avoid these errors in careful mgmt of this role.

Agree. My thoughts were the same, either enforce this on MasterDeployer, or expect that we'd never add a pool type with the same identifier but with different logic (this is why versioning would be nice), none the less there's potential for human error to creep in here which is why the point was made.

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 a pull request may close this issue.

2 participants