refactor(solecs): use @solidstate Ownable, add OwnableWritable, fix some import formatting #265

dk1a · 2022-11-25T20:36:23Z

Closes #245

Use Ownable from @solidstate/contracts to implement IERC173, also use solidstate for IERC165
Add IOwnableWritable and OwnableWritable on top of IERC173 and Ownable, to generalize authorization which was previously built into components
Add Subsystem, which is a System, but uses OwnableWritable instead of Ownable
Fix some import formatting since I was going through the whole package anyways

Edit: removed paragraph about subsystems since they're moved to #268

cha0sg0d · 2022-11-26T12:05:58Z

cc @Kooshaba, this touches on some of your multiple system contract ideas i think,

alvrs · 2022-11-26T18:19:44Z

What's the benefit of Subsystems over Libraries?
Could you go into some more detail behind the reasoning of introducing diamond storage (+ benefits, implications)?

Also, let's split up the Ownable refactor and the Subsystem changes into two separate PRs

dk1a · 2022-11-26T19:03:25Z

Removed Subsystem from this PR, will elaborate on it in its own PR.

Diamond storage:

benefits:

Libraries can directly and easily use diamond storage (something like feat(solecs): add deterministic storage location for systems, add storage access util #264). When storage slots are sequential, inheritance can make them unpredictable or difficult to work with.
Having it out of namespace removes some ambiguity, like in constructor arguments (but that's just my personal preference).
Upgradeability, but it is a very minor concern here, usually it's easier to just register new system/component over old one. And owner alone doesn't solve upgradeability issues anyways.

implications:

Some project clutter, since diamond storage requires an extra file.
Technically a breaking change (but won't affect most users, directly using _owner wasn't usually necessary, and owner(), transferOwnership() behave the same)
Using owner directly in the contract will be fiddly, having to import the storage file (but as with the previous point, that should almost never be needed)

The first benefit applies even more so to the other state vars, especially in System: world, components. But that's for another PR (the 2 drafts I made)

dk1a · 2022-11-26T19:07:14Z

Oh also removed the tests, they'll be together with Subsystem since they use it, unless it's rejected changing them back and forth seems unnecessary.

alvrs

Looking really good, thank you! Just had one minor comment and one question, and need to test locally.

alvrs · 2023-01-05T12:13:43Z

packages/solecs/src/OwnableWritable.sol

+    if (!writeAccess(msg.sender)) {
+      revert OwnableWritable__NotWriter();
+    }


curious about your preference of revert over require - could you elaborate?

it's more about my preference of custom errors over error strings. Custom errors just don't support require, unfortunately.
I took the specific semantics (Contract__ErrorName) from solidstate.

Unlike strings, custom errors are guaranteed to be devoid of typos, since you can define them in one place, and use (via imports or inheritance) everywhere

They allow args (like events), which is the most convenient way to do dynamic errors imo

They aren't strings, so they should be more gas-efficient

I just don't like the hardcoded nature of error-strings (this is similar to the 1st point, but about personal preference rather than some objective benefit)

imo it'd be great if mud started leaning towards these event-like errors. But if you ultimately prefer require, I can keep that in mind for future PRs

Seconding request for use of Custom Errors moving forward!

Thanks for elaborating, I agree custom errors are a better approach!

packages/solecs/src/OwnableWritableStorage.sol

alvrs · 2023-01-05T13:39:30Z

Seems like @solidstate/contracts removed setOwner from OwnableStorage since v0.0.49: solidstate-network/solidstate-solidity@7378a2c. Should we upgrade to the latest version (0.0.52)?

dk1a · 2023-01-05T16:36:04Z

Seems like @solidstate/contracts removed setOwner from OwnableStorage since v0.0.49: solidstate-network/solidstate-solidity@7378a2c. Should we upgrade to the latest version (0.0.52)?

Updated it. I actually prefer the lack of setOwner

alvrs

Looks great! We should make sure to mention the requirement to install @solidstate/contracts in consuming packages in the merge commit. Technically a breaking change, but not worth waiting for this change until the next breaking version imo.

holic · 2023-01-10T16:28:15Z

packages/solecs/src/Ownable.sol

+contract Ownable is SolidStateOwnable {
+  constructor() {
+    // Initialize owner (SolidState has no constructors)
+    OwnableStorage.layout().owner = msg.sender;


ooc why set this manually like this rather than calling _setOwner(msg.sender)?

Because it's set in just 1 place and having an explicit method for it seemed unnecessary.
The method also emits an event.
Which is important, because after checking EIP-173, the event should be emitted on creation too.
Since it's merged I'll make a fix PR to use _setOwner, @holic thanks for noticing

Because it's set in just 1 place and having an explicit method for it seemed unnecessary.

By that I mean, _setOwner didn't exist in the previous version (I made the PR 2 months ago and recently updated solidstate), and when updating I didn't think to check for a method

#265 (comment)

and this comment is about another setOwner which was removed, it didn't emit an event

@SolidState

…ome import formatting (latticexyz#265) * refactor(solecs): use @SolidState Ownable, add OwnableWritable, fix some import formatting (latticexyz#265) * build(solecs): add peerDependencies to package.json breaking: - consumers need to install `@solidstate/contracts` as a new peerDependency of `solecs`. - internal `_owner` field was removed from `Component`. (Use `owner()` instead.) Co-authored-by: alvrs <alvarius@lattice.xyz>

dk1a requested a review from alvrs as a code owner November 25, 2022 20:36

dk1a mentioned this pull request Nov 25, 2022

feat(solecs): add deterministic storage location for systems, add storage access util #264

Merged

dk1a mentioned this pull request Nov 26, 2022

feat(solecs): add AbstractComponent #267

Closed

dk1a force-pushed the dk1a/access_control branch from 72c9383 to 97405f7 Compare November 26, 2022 18:35

dk1a changed the title ~~feat(solecs): refactor access control, add Subsystem, fix some import formatting~~ feat(solecs): refactor access control, fix some import formatting Nov 26, 2022

dk1a mentioned this pull request Nov 26, 2022

Add subsystem #268

Closed

dk1a mentioned this pull request Dec 10, 2022

Registration ids are global #287

Closed

dk1a force-pushed the dk1a/access_control branch from 97405f7 to 3c7a7f8 Compare December 19, 2022 17:43

dk1a mentioned this pull request Jan 2, 2023

feat(cli): add support for subsystems in writeAccess to LibDeploy #301

Closed

alvrs reviewed Jan 5, 2023

View reviewed changes

alvrs force-pushed the dk1a/access_control branch from 3c7a7f8 to 861ee0f Compare January 5, 2023 12:22

alvrs self-assigned this Jan 5, 2023

alvrs added the changes requested label Jan 5, 2023

dk1a and others added 6 commits January 10, 2023 16:21

feat(solecs): refactor access control, fix some import formatting

3252398

build: add support for trailing new line in remappings.txt

d7de6b6

chore: change license to MIT

7b3191d

chore(solecs): update solidstate

4de5699

chore(solecs): remove obsolete using for

93fb4d9

build(solecs): add peerDependencies to package.json

43d075d

alvrs force-pushed the dk1a/access_control branch from 30bd3c4 to 43d075d Compare January 10, 2023 15:21

alvrs approved these changes Jan 10, 2023

View reviewed changes

alvrs changed the title ~~feat(solecs): refactor access control, fix some import formatting~~ refactor(solecs): use @solidstate Ownable, add OwnableWritable, fix some import formatting Jan 10, 2023

alvrs merged commit 4043f3c into latticexyz:main Jan 10, 2023

holic reviewed Jan 10, 2023

View reviewed changes

dk1a mentioned this pull request Jan 10, 2023

fix(solecs): use _setOwner in Ownable constructor #338

Merged

dk1a deleted the dk1a/access_control branch February 5, 2023 10:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(solecs): use @solidstate Ownable, add OwnableWritable, fix some import formatting #265

refactor(solecs): use @solidstate Ownable, add OwnableWritable, fix some import formatting #265

dk1a commented Nov 25, 2022 •

edited by alvrs

Loading

cha0sg0d commented Nov 26, 2022

alvrs commented Nov 26, 2022

dk1a commented Nov 26, 2022

dk1a commented Nov 26, 2022

alvrs left a comment

alvrs Jan 5, 2023

dk1a Jan 5, 2023 •

edited

Loading

cha0sg0d Jan 5, 2023

alvrs Jan 5, 2023

alvrs commented Jan 5, 2023 •

edited

Loading

dk1a commented Jan 5, 2023

alvrs left a comment

holic Jan 10, 2023 •

edited

Loading

dk1a Jan 10, 2023

dk1a Jan 10, 2023

dk1a Jan 10, 2023

refactor(solecs): use @solidstate Ownable, add OwnableWritable, fix some import formatting #265

refactor(solecs): use @solidstate Ownable, add OwnableWritable, fix some import formatting #265

Conversation

dk1a commented Nov 25, 2022 • edited by alvrs Loading

cha0sg0d commented Nov 26, 2022

alvrs commented Nov 26, 2022

dk1a commented Nov 26, 2022

dk1a commented Nov 26, 2022

alvrs left a comment

Choose a reason for hiding this comment

alvrs Jan 5, 2023

Choose a reason for hiding this comment

dk1a Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

cha0sg0d Jan 5, 2023

Choose a reason for hiding this comment

alvrs Jan 5, 2023

Choose a reason for hiding this comment

alvrs commented Jan 5, 2023 • edited Loading

dk1a commented Jan 5, 2023

alvrs left a comment

Choose a reason for hiding this comment

holic Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

dk1a Jan 10, 2023

Choose a reason for hiding this comment

dk1a Jan 10, 2023

Choose a reason for hiding this comment

dk1a Jan 10, 2023

Choose a reason for hiding this comment

dk1a commented Nov 25, 2022 •

edited by alvrs

Loading

dk1a Jan 5, 2023 •

edited

Loading

alvrs commented Jan 5, 2023 •

edited

Loading

holic Jan 10, 2023 •

edited

Loading