Migrate SRC5 to component

[ericnordelo]

Supersedes #755 Fixes #754

PR Checklist

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

[andrew-fleming]

Great work, Eric! I left a few questions. And as mentioned in our discussion, I vote that we update the relevant documentation with each component migration PR

[andrew-fleming]

Improvements look good, sir! I left a couple suggestions and a question :)

[andrew-fleming]

Should we change the self type in the functions below to ComponentState<TContractState>?

[ericnordelo]

What do you think about leaving just ComponentState? I think the generic TState is implicit since TContractState is always required, and this will appear frequently in the APIs.

[andrew-fleming]

Good call, and it's much less noisy. +1

[ericnordelo]

Actually, I think we should leave just ContractState, because the embeddable implementation generated when you use embeddable_as is the one having just ContractState, like this:

#[starknet::embeddable]
impl SRC5Impl<
            TContractState, +HasComponent<TContractState>
, impl TContractStateDrop: Drop<TContractState>
> of interface::ISRC5<TContractState> {
    
    fn supports_interface(self: @TContractState, interface_id: felt252
) -> bool {
        let component = HasComponent::get_component(self);
        SRC5::supports_interface(component, interface_id, )
    }

[ericnordelo]

The impl we embed is not the one we write in the component, but the one the compiler generates from the embeddable_as attribute. And this is the one the user would usually use. Is different for InternalImpl though, because this doesn't use embeddable_as. For this one I think we should use ComponentState.

[andrew-fleming]

Hmm if we go that route, should we include a NOTE or something to explain the rationale behind using ContractState for embeddables and ComponentState for internals? IMO it'd be weird to not say anything about it

[ericnordelo]

I agree it can be confusing, but where can we add such note? It seems we would need to add it to every component API

[andrew-fleming]

Repeating the same note is definitely not ideal. Even though this is about reading the API, it's IMO more about understanding the component architecture (as you described above). Maybe this can be addressed in the Extensibility doc

[ericnordelo]

Agree, let's stick with ContractState for embeddable impls functions, and ComponentState for InternalImpl in components, at least until we revisit how we present this in the Extensibility PR. wdyt?

[andrew-fleming]

Sounds good to me

[andrew-fleming]

I left an optional comment to address and a couple small suggestions. Otherwise, LGTM!

[andrew-fleming]

Regarding the title: Embeddable Implementations Functions, it reads kind of awkward. This is nitpicky, I know. I don't have a good alternative, just throwing it out there

[ericnordelo]

Let's call it just Embeddable Functions in the meantime (I think it reads better). Any suggestion is more than welcome.

[andrew-fleming]

In the Ownable API, the index name is Embeddable Implementations Functions. We should normalize

[ericnordelo]

Let's stick with just Embeddable Implementations then if you agree.