Migrate account presets to camelCase

[martriay]

Fixes #382

[andrew-fleming]

Looks great, @martriay! I left a small suggestion. Also, should we update the IAccount magic value in the constants lib (and subsequently in the docs)?

[andrew-fleming]

I think we need to camelCase the return is_valid here as well

[juniset]

@martriay The more I think about this the less I think it is a good idea. I started to align in the Argent repo then realized it posed a lot of questions: should I use camelCase for external function parameters as well? Output parameters? The answer is probably yes if you want to be consistent (weird to have both camel case and snake case in the same function definition), but then you are passing camel case variables to your snake case internal methods... and you are back to the same mixing mess. Not even mentioning the fact that __execute__ and __validate__ still have snake case arguments.

While I can understand the logic of using camel case for ERC standard, the IAccount interface is purely a StarkNet standard so the argument does not apply there. On the other hand, I feel that you are gradually diverging from the convention of all the other projects in the ecosystem which will either force everybody to align for consistency (in that case it should probably be discussed), or make all the other projects inconsistent by forcing them to mix between different conventions (e.g. when they interact with an account).

[martriay]

Hello @juniset thanks once again for giving this a thorough thought.

should I use camelCase for external function parameters as well? Output parameters?

Yes. All external methods and its parameters would be camelCased as per the extensibility pattern. We've received many questions/complaints around us not following it and the confusion this generates. We agreed with them.

While I can understand the logic of using camel case for ERC standard, the IAccount interface is purely a StarkNet standard so the argument does not apply there.

The extensibility pattern tries to provide easy to follow guidelines to design libraries and contracts. It does not reference EIPs or other networks, it's purely StarkNet.

Although it recognizes the existence of EIP-based interfaces, it's not the goal to specify how to deal with them but to simplify the thought process so developers don't have to think whether they're interacting/interfacing with an ERC standard to name their functions, so it simply uses camelCase for contracts (similar to Vyper's decision).

Part of the reason for this is that there's a lot of non-EIP but defacto standards such as access control interfaces, or you could even be implementing an interface that ends up being an EIP in the future. EIP-ness of an interface is not future proof.

The goal therefore is to have a super simple mental model: if it's an external/public function of a contract, it needs to be camelCase. You don't need to worry about deciding/guessing whether it corresponds to an existing (or future!) EIP or not.

then you are passing camel case variables to your snake case internal methods... and you are back to the same mixing mess. Not even mentioning the fact that __execute__ and __validate__ still have snake case arguments.

I recognize there's 2 exceptions here: system-defined account entrypoints (__execute__, __validate_declare__, and __validate__) as well as system-defined parameters (N_len for any N named array). I see accepting these exceptions as super specific the lesser evil. Otherwise we'll see a huge mix casing when calling contract functions (and I personal believe that (1) default entrypoints won't be much called from contracts, and (2) that N_len parameters will be absorbed by the language in the future, as any other high level language).

There's one super important point to highlight here: ecosystem-wide consistency was not possible since the moment starkware decided to use snake_case for their stdlib (unlike Vyper), and users wanting to follow ERC interfaces. Unless one of them gets dropped, we can't have consistency.

Once we accept inconsistency, the question is what rule/pattern are we going to use to decide when to use each casing. Here's is where we lean towards the simpler model: all external interfaces have to be camelCase (an easier superset of camelCasing ERC contracts only). Distinguishing between non/ERC contracts is a much harder thought process with no clear gain, since we'll have inconsistencies anyway. This decision only minimizes those inconsistencies.

On the other hand, I feel that you are gradually diverging from the convention of all the other projects in the ecosystem which will either force everybody to align for consistency (in that case it should probably be discussed), or make all the other projects inconsistent by forcing them to mix between different conventions (e.g. when they interact with an account).

Considering the points presented above, I believe the ecosystem is already diverging by itself. Some projects use cameCase for their externals, some projects use ERC contracts, etc. If we were to not implement camelCase on the Account, we would not be following the Extensibility pattern thus collaborating to the growing chaos.

Casing mixing already exists and this change is our own take on how to minimize it, using some form of occams' razor: if in doubt, choose the simpler model.

Re: discussion, there were discussions for this (here and here) and if anything they just showed lack of ecosystem-wide agreement, for which we need to reply with an opinionated decision otherwise we'll stall.

Finally, considering that:

• at this point we're looking into releasing Cairo 0.10 support as soon as possible and we're in the final lap
• with the 0.10 migration and eventual regenesis coming soon, we aim to bundle as many breaking changes together in a single release as possible.

Is that we sadly cannot afford to reopen this discussion so close to releasing.

[martriay]

This PR is superceded by #449 btw.

[milancermak]

There is ecosystem wide agreement on naming now - it's snake_case. Look at Aave, Jedi swap, Starknet ID, Dope Wars, Empiric Oracle just to name a few. From my point of view, it's OZ that's diverging from the consensus. Besides ERC methods, there really not much support for camelCase anywhere.

at this point we're looking into releasing Cairo 0.10 support as soon as possible and we're in the final lap

This change didn't really have to be in the 0.10 support release, you decided so.

I'm curious, did you consult any of the leading wallet providers (Argent, Braavos, Ledger, MetaMask, apologies if I'm missing any) about this breaking change?

Finally, won't changing this break the StarkNet CLI?

[martriay]

This change didn't really have to be in the 0.10 support release, you decided so.

Milan your tone has been way too confrontational in all discussions so far, even when you were mistaken like in here. I'd like to gently ask you to change it if you're interested in positive discussion.

This change was planned for this release as you can see the issue that this very PR aims to close, is part of the same release as Cairo 0.10. The comment you're linking is just saying we should close that issue in the same PR instead of a different one.

I'm curious, did you consult any of the leading wallet providers

We did. Julien himself expressed his concerns but accepted the decision a while ago. Now he's been working on it and had second thoughts, but it was agreed.

Finally, won't changing this break the StarkNet CLI?

Good point, we'll have to coordinate with them although probably they were going to include it anyway since they use our implementation and we didn't release one for 0.10 yet.

[martriay]

This change didn't really have to be in the 0.10 support release, you decided so.

And yes I decide what's on the roadmap because that's my job. But we've been super public about our roadmap and design, more than any other project in the space. And the rationale behind my decision is above: to bundle as many breaking changes together as possible, to minimize migration hurdles.

We have to make decisions. Some of them good, some of them bad. We aim to maximize the former, minimze the latter. But it's not easy, especially when there's no ecosystem wide agreement and I understand you feel very strongly about your opinion, but it's not enough to legitimize it. We are forced to make a decision and I know of very few projects taking community input as we do, but this decision has been made and the rationale is above.

Unless a very big reason appears (which has not), we cannot afford to change plans 1 or 2 days before releasing, I'm sorry. You're still on time to discuss what's happening on the next release, and the one after that.

[milancermak]

You are right. Even though I follow this repo quite closely, I don't remember seeing the project management UI and I missed that the IAccountchange was assigned to be in this upcoming release 2 weeks ago. I missed that, my bad and I apologize.

Also note it's nothing personal, I'm simply trying to stand up for what I believe is right.

I understand you feel very strongly about your opinion, but it's not enough to legitimize it.

Yes, I do have a strong opinion as do you. So that applies both ways☝🏼

when there's no ecosystem wide agreement

That's simply not true. Cairo stdlib is snake case, so there's precedence. Furthermore, I've presented a handful of well known projects above, all using snake case in their API.

From my point of view, this change doesn't bring any benefit to the ecosystem.

[juniset]

Thanks @martriay for the detailed response. And I'm sorry to have reopened the discussion, just realised some implications of it while working on its implementation :-)

@milancermak I want to emphasise that Martin has been very transparent and communicative about this change, and we started the discussion a while ago. While I don't necessarily agree with it, I do recognise that it makes sense for the consistency of the OZ repo since their starting point is to write ERC compliant contract standards on StarkNet. And I did agree to it for the sake of consistency.

But we need to communicate that change to the wider ecosystem because is_valid_signature is used by dapps. So we'll need to update the libraries to use the camel case version (starknet.js, starknet.py, etc) as well as tell dapps to update their contracts. Changing the account interface is a breaking change.

I might keep both versions (is_valid_signature and isValidSignature) to give dapp contract the time to adapt.

[martriay]

Also note it's nothing personal, I'm simply trying to stand up for what I believe is right.

I know and I appreciate you being on top of our development process, you made notable contributions in the past so I'm glad to know we can keep it civil.

Yes, I do have a strong opinion as do you. So that applies both ways☝🏼

Sure that's what I mean by lack of ecosystemic agreement but as I said before decisions have to be made and I'm in a position that forces me to do so. I know I'll bear the weight of bad decisions and this time I think this is the right way.

Thanks @martriay for the detailed response. And I'm sorry to have reopened the discussion, just realised some implications of it while working on its implementation :-)

Yes, very understandable.

But we need to communicate that change to the wider ecosystem because is_valid_signature is used by dapps.

I think this is one of Milan's most accurate points. I started to reach out to projects so they update accordingly. I also sent an updated comment on the standard account interface discussion, and I'll open an issue on the shamans forum.

Closing this now since it's been superceded by #449. Thanks again for your comments.