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

Refactor Ownership Library for better UX #204

Merged
merged 6 commits into from
Nov 21, 2023
Merged

Conversation

bitzoic
Copy link
Member

@bitzoic bitzoic commented Nov 16, 2023

Type of change

  • Improvement (refactoring, restructuring repository, cleaning tech debt, ...)

Changes

The following changes have been made:

  • Swaps the need for declaring ownership in storage for a pre-defined StorageKey to store storage. This results in the following improvements in developer UX as the use of the library was previously very verbose.

Before:

storage {
     owner: Ownership = Ownership::initialized(Identity::Address(Address::from(ZERO_B256))),
}

fn foo() {
     let owner = storage.owner.owner();
     storage.owner.only_owner();
     storage.owner.renounce_ownership();
}

After:

fn foo() {
     let owner = _owner();
     only_owner();
     renounce_ownership();
}

The downside to this approach is the inability to set the owner during compile time and a constructor must be implemented.

@bitzoic bitzoic added Enhancement New feature or request Lib: Access Label used to filter for the library issue Breaking This will break user's code labels Nov 16, 2023
@bitzoic bitzoic self-assigned this Nov 16, 2023
@bitzoic bitzoic requested a review from a team as a code owner November 16, 2023 08:37
Copy link
Contributor

@Braqzen Braqzen left a comment

Choose a reason for hiding this comment

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

Before I review the changes themselves can you elaborate on the technical need for this change?

To me it seems like a subjective change in UX.
I think that having the ability to set data at compile time is important and useful. At the same time being able to delay that via a constructor may be useful as well.

Without deeper consideration at this stage I'll await your response and instead superficially propose the idea of having 2 ownership standards. One for configuration and one for construction. That has trade-offs as well but I don't think that's as much of an issue as providing one UX over the other.

@bitzoic
Copy link
Member Author

bitzoic commented Nov 17, 2023

Before I review the changes themselves can you elaborate on the technical need for this change?

To me it seems like a subjective change in UX. I think that having the ability to set data at compile time is important and useful. At the same time being able to delay that via a constructor may be useful as well.

Without deeper consideration at this stage I'll await your response and instead superficially propose the idea of having 2 ownership standards. One for configuration and one for construction. That has trade-offs as well but I don't think that's as much of an issue as providing one UX over the other.

This is something I considered, but there are a few caveats to this.

  1. In the future we expect to have the ability to import storage from libraries, thus enabling the ability to set the owner at compile time
  2. With most developers coming from Solidity, a constructor is something they are already accustomed to
  3. If a developer chooses to set the owner at compile time, they can do so using just the SRC-5 standard. This library is intended for convenience and is by no means the only way to implement the standard

I believe the proposed changes make Sway code significantly more readable and less verbose. Having to call storage.owner.my_function() for absolutely everything simply doesn't make sense compared to just the function itself. The trade off being well worth the minor inconvenience of a constructor in turn for easier auditability.

@bitzoic bitzoic requested review from Braqzen and a team November 17, 2023 07:39
K1-R1
K1-R1 previously approved these changes Nov 17, 2023
Copy link
Member

@K1-R1 K1-R1 left a comment

Choose a reason for hiding this comment

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

Big fan of the UX improvements here. Just a couple of comments for the README, but they're only suggestions

libs/ownership/README.md Outdated Show resolved Hide resolved
libs/ownership/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@Braqzen Braqzen left a comment

Choose a reason for hiding this comment

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

<3

@bitzoic bitzoic merged commit c1813e1 into master Nov 21, 2023
4 checks passed
@bitzoic bitzoic deleted the bitzoic-update-ownership branch November 21, 2023 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking This will break user's code Enhancement New feature or request Lib: Access Label used to filter for the library issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants