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: use interface over imported registry in deployments helper #475

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

dtbuchholz
Copy link
Contributor

Summary

Alters TablelandDeployments to use a wrapper/interface around the ITablelandTables and IERC721AUpgradeable contracts, enabling the same functionality while resolving potential dependency issues. Originally noted here and here (happened to at least two developers).

Details

Currently, the TablelandDeployments contract imports TablelandTables, which is an OpenZeppelin OwnableUpgradeable contract. This can cause issues because, for example, Remix installs dependencies but doesn't resolve peer-dependencies by grabbing the version the first library depends on—it just resolves to the latest version. With OZ's v5 release, the OwnableUpgradeable contract has breaking changes, as do some others like PausableUpgradeable that now exist in different package path.

Remix is still a highly used entrypoint for developers testing out smart contracts, so making things fully compatible here makes sense. Alternatively, we could change TablelandTables to use OZ v5 contracts, but the TablelandDeployments change seems easier.

How it was tested

  • Existing tests passed.
  • Tested out with example contract in Remix.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Copy link
Member

@sanderpick sanderpick left a comment

Choose a reason for hiding this comment

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

nice, make sense and looks good to me

@joewagner
Copy link
Contributor

joewagner commented Jan 3, 2024

Will this affect the issue that was fixed in #448 ?

Specifically it looks like you might be affectively reverting this commit: 8a73d4a

@dtbuchholz
Copy link
Contributor Author

dtbuchholz commented Jan 3, 2024

@joewagner it shouldn't have any impact! That branch was a feature that changed the TablelandDeployments behavior from using a registry interface to using the actual registry contract, which opened up methods that aren't explicitly part of the registry interface (like token transfers). However, that had side effects that this PR aims to address.

Wrt that commit--correct, the logic in this PR, effectively, reverts that commit. Initially, I had implemented the TablelandTablesImpl interface/wrapper in that PR but changed to directly importing the TablelandTables contract. This was purely a simplicity decision since importing TablelandTables was a single import, but the wrapper/interface setup accomplishes the same thing and avoids the dependency side effects.

@dtbuchholz dtbuchholz merged commit 7ef0193 into main Jan 3, 2024
2 checks passed
@dtbuchholz dtbuchholz deleted the dtb/deployments-impl branch January 3, 2024 20: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.

3 participants