Apply underscore pattern

[ericnordelo]

Fixes #795

The proposed pattern is the following:

• Avoid underscore by default, except in the following cases:
• When the function name collides with a function name outside the InternalImpl (e.g. approve vs _approve).
• When the function is intended as private, which is not meant to be used outside the component (it can be used in presets but special care should be taken) (e.g: _approve_with_optional_event).

PR Checklist

• Tests
• Documentation
• Added entry to CHANGELOG.md
• Tried the feature on a public network

[andrew-fleming]

Nice work on the proposal! I agree with most of it; however, I'm not sure on this one:

When the function name collides with a potential external implementation even when is not directly provided, to avoid potential confusion (e.g. _upgrade in the UpgradeableComponent or _pause in the PausableComponent).

You mean a function within an implementation, right? IMO it's unnecessary because accessing those functions in a contract need to include the component's state i.e.

fn pause(self: @ContractState) {
    self.pausable.pause();
}

I don't think this will be harder for users to follow than self.pausable._pause()

It's not the worst thing if we keep this condition, but the less exceptions to the underscore convention, the better. Note that one could also argue that mint and burn, for example, could be functions in a contract as well (hence should be underscored). What are your thoughts?

[ericnordelo]

I agree and will update accordingly. Thanks @andrew-fleming. BTW, I forgot to mention that this PR also changes the order of some functions in the internal implementation and is prioritizing not-underscored-functions in the order.

[andrew-fleming]

Most of the components look good with the changes! I left a few questions

As an aside, I think implementing #994 will further compliment this approach well

[andrew-fleming]

Should we change _mint to mint since we're doing that with erc20?

[ericnordelo]

Similar to _transfer in ERC721, I think we should keep _mint underscored because we don't want users to use them, those are private helpers for the "safe" versions that are not underscored (safe_mint and safe_transfer)

[andrew-fleming]

Similar to _transfer in ERC721, I think we should keep _mint underscored because we don't want users to use them

My impression was that we were taking an agnostic stance regarding x and safe_x because there's some risk in both approaches. They'll still be accessible and this is more of an organizational change, so it's not the end of the world if you prefer it this way. I'm just not a fan of blanket-favoring the safe variants

[ericnordelo]

mmm, that's a very good point. Agree.

[andrew-fleming]

Should _transfer be transfer in ERC721? The underscore is consistent with ERC20 though...idk. Thoughts?

[andrew-fleming]

Also, should we make _upgrade => upgrade in Upgrades?

[ericnordelo]

Should _transfer be transfer in ERC721? The underscore is consistent with ERC20 though...idk. Thoughts?

The answer is in the previous comment.

[ericnordelo]

I changed _update to update in ERC721 because we want users using that, for example, in the wizard, it is directly used to implement burn.

[andrew-fleming]

We're almost there! I left a few comments

[andrew-fleming]

We're missing _update in the API

[andrew-fleming]

And forgive me, I should have asked this before 😅 would it make sense to remove the underscore in _update for erc20 and erc1155? It seems weird that we're only doing it with erc721. The rationale, as I see it with erc1155, is to promote update_with_acceptance_check. I can get behind that if you want to keep it, but I'm not so sure with erc20. For consistency, we could just do update across the tokens. WDYT?

[ericnordelo]

I think is ok to remove them.

[andrew-fleming]

We should switch mint with transfer to maintain the correct order

[ericnordelo]

Fixed in the links section, since the order in the code is transfer before mint.

[andrew-fleming]

This should be between burn and _owner_of

[andrew-fleming]

Final (non-blocking) suggestion: we should remove an underscore from the double-underscored erc20 and erc721 test names i.e. test__mint, test__burn, etc. Other than that, I think we're good to go!