Migrate Ownable to component

[ericnordelo]

Supersedes #752 Fixes #753

PR Checklist

• Tests
• Tried the feature on a public network
• Documentation

[enitrat]

Hey!
Just popping into the convo as I was working myself on creating an Ownable component and wanted to share something cool with you about component testing.

I see that you've created a mock contract as well that allows you to get a concrete ContractState for a component. I've done the same thing, but instead of interacting with the component through the contract, by using state.ownable, I've managed to interact directly with the internals of the components by passing the component state.

An example is worth a thousand words:

/// Mock contract embedding ownable_component here ...

/// Assuming a component and mock contract with a logic similar to yours 
use ownable_component::{InternalImpl, OwnableImpl};
type TestingState = ownable_component::ComponentState<MockContract::ContractState>;

// You can derive `Default` on this type alias
impl TestingStateDefault of Default<TestingState> {
    fn default() -> TestingState {
        ownable_component::component_state_for_testing()
    }
}

// Or even implement your own trait for helper functions
#[generate_trait]
impl TestingStateImpl of TestingStateTrait {
    fn new_with(owner: ContractAddress) -> TestingState {
        let mut ownable: TestingState = Default::default();
        ownable.initializer(owner);
        utils::drop_event(ZERO());
        ownable
    }
}


// and then use it like this:
#[test]
#[available_gas(200000)]
fn test_ownable_initializer() {
    let mut ownable: TestingState = Default::default();
    assert(ownable.owner().is_zero(), 'owner should be zero');

    ownable.initializer(OWNER());

    assert_event_ownership_transferred(ZERO(), OWNER());
    assert(ownable.owner() == OWNER(), 'Owner should be set');
}

// or like this

#[test]
#[available_gas(2000000)]
fn test__transfer_ownership() {
    let mut ownable: TestingState = TestingStateTrait::new_with(OWNER());

    ownable._transfer_ownership(OTHER());

    assert_event_ownership_transferred(OWNER(), OTHER());
    assert(ownable.owner() == OTHER(), 'Owner should be OTHER');
}

anyway let me know what you think! It's very similar but I thought it was cool.

[ericnordelo]

Hi @enitrat, this looks great to me. Thanks for sharing!

Currently, the plan is to migrate to components trying to avoid rewriting the test suite as much as possible. This idea is certainly not a big rewrite, but it involves some design, and my only concern (from a quick review) is that using the mock state directly is probably a better approach when testing components that depend on other components (like AccessControl depending on SRC5). I would try to merge the PRs with the current design and revisit this shortly after to refactor where it applies, just to avoid potentially delaying the milestone, what do you think @andrew-fleming?

[andrew-fleming]

I feel a strong urge to use contract.fn instead of state.contract.fn!

I agree with Eric, though. Let's get the need-to-haves done and then we can enhance our testing strats thereafter. I created an issue #777. Thanks for the idea, @enitrat!

[enitrat]

my pleasure! Keep up the good work guys 🙌

[andrew-fleming]

Nice work, Eric! I left a few questions and suggestions

[andrew-fleming]

Based on #761 (comment), should we normalize CAMELCASE-SUPPORT for consistency or only use that for multiple camel impls?

[ericnordelo]

mmm, I know we agreed on this, but thinking about this now, I'm having second thoughts. How will users find the name of the Impl to embed for exposing the Camel API if we don't add the name somewhere in the API doc? Contracts using components need to explicitly embed the component implementations.

[ericnordelo]

I lean towards adding explicitly mentioning the name of the impl in components, so users can now what to embed.

[andrew-fleming]

Thinking of how users will use the component, IMO it'd be a nice option to also have a single ContractABIImpl which will include all the ABI functions. It's not a huge deal with Ownable, but ERC20, for example, will have four embeddable impls:

1. ERC20Impl
2. ERC20NonStandardImpl
3. ERC20CamelOnlyImpl
4. ERC20CamelNonStandardImpl

Thoughts?

[ericnordelo]

I see your point, but the issue I have with it is that I think users should have the ability to opt-in/out of some impl for components, and having a single big ABI enforces every function by default. Examples of components that I think should have multiple external impl are:

ERC721: the ERC721Metadata impl should be optional, even when is widely used. The standard doesn't require this, and users should be able to not use it IMO.

The ERC20 example as well should have the metadata part as optional, but also the non-standard impl IMO.

I think I lean towards letting multiple impl, even when I feel (agree) this could be improved somehow.

[andrew-fleming]

I see your point, but the issue I have with it is that I think users should have the ability to opt-in/out of some impl for components, and having a single big ABI enforces every function by default.

Oh, for sure! I was thinking more along the lines of having the implementations that we have now and also one big ABI impl with everything. This gives the user the option to either choose specific impls or use the whole component. I imagine this being quite beneficial for the account and erc721 components

The issue I see, though, is we'd have to rewrite all the methods under the ABI impl or create some sort of multi-impl embeddable mechanism

[ericnordelo]

I lean towards modularity here, having ERC721, ERC721Metadata, and non-standard (with a more descriptive name, not generic) impls separately and required explicitly in the contract. Having an extra impl with the full ABI seems like an antipattern, because it would be harder to follow IMO.

[andrew-fleming]

Improvements look great! I left a couple suggestions

Also, the Ownable link in access.adoc is broken (line 1). It's missing the v in 0.7.0

[andrew-fleming]

Suggested change

	/// camelCase support for `transfer_ownership`.
	fn transferOwnership(ref self: ComponentState<TContractState>, newOwner: ContractAddress) {
	self.transfer_ownership(newOwner);
	}
	/// camelCase support for `renounce_ownership`.
	/// Camel case support for `transfer_ownership`.
	fn transferOwnership(ref self: ComponentState<TContractState>, newOwner: ContractAddress) {
	self.transfer_ownership(newOwner);
	}
	/// Camel case support for `renounce_ownership`.

I know we want to be consistent with camelCase, but when it starts a sentence, I vote we separate the words and capitalize Camel. It looks odd otherwise IMO

[andrew-fleming]

Or we can rephrase the sentence and use camelCase to avoid this situation

[ericnordelo]

Re-phrased, let me know what you think

[andrew-fleming]

Looks good! Since we're consolidating, you think it's worth adding a link to the dual interface doc?

[andrew-fleming]

Left a final suggestion, but I think it's good to go!

[andrew-fleming]

Much better!