Use `assert_valid` instead of `is_valid`

[pscott]

Motivation

Making the account API more intuitive by using assert_valid_signature instead of is_valid_signature

Current implementation

Currently, is_valid_signature does not return anything but will fail if the signature is invalid. I believe this is counter intuitive because the is_ prefix can let the user think it will return a 1 or 0.

Requested implem

The reason why this function throws is because of its underlying call to verify_ecdsa_signature. Since we can't fix that, I believe we should change the naming of is_valid_signature to assert_valid_signature (similarly to all the different assert_... in the common.math library.

Additional information

As discussed with @juniset (in pm), the rationale behind using is_valid_signature was to be compatible with EIP 1271 (using snake case), by hoping we would one day be able to return the flag (maybe Starkware would modify verify_ecdsa_signature and we would have this EIP support right off the bat.

IMO, doing that will lead to different issues:

1. We don't really have compatibility (since we're not doing the exact same thing as the EIP specifies...)
2. It's counter intuitive (I was surprised is_ doesn't return a flag)
3. We will need to introduce breaking changes anyway because devs will need to adapt their code to verify the return flag.

In light of those different issues, I think using assert_ not is a good idea, because when/if verify_ecdsa_signature returns a flag, we will be able to keep assert_is_valid, introduce is_valid and will lead to less "breaking changes" than the other solution! :)

[martriay]

Agree

[juniset]

I don't think removing is_valid_signature is a good idea, particularly for a question of temporary semantic. It is part of the IAccount interface and changing that interface will create chaos on our growing ecosystem. Every library, wallet, dapp will need to be updated and it will require a good amount of coordination to do so without impacting existing users (updating OZ is not the same as updating the Argent implementation where we have more than 100k users, some of them with real ETH...). Given the coming breaking changes of Cairo 0.9 and 0.10 to existing accounts I don't see a good reason to create yet another breaking change for that.

Note that is_valid_signature is not meant to be exactly compatible with EIP1271, but to be aligned with this standard such that devs have a common naming for the interface of accounts on chains supporting account abstraction (or just smart-contract wallets). For example accounts on zksync v2 will have a version of is valid signature (snake case or camel case).

Given the above I do believe is_valid_signature is the right method for accounts on StarkNet. And it was always intended to return a flag. The fact that it does not is only because of the current limitations of Cairo but we can hope to have these limitations removed soon.

StarkNet is very early and I think our priority should be on having common interfaces on which we can all build for the future, even if the implementations are not yet 100% compliant with these interfaces.

As an alternative the method could return 1 when it does not revert.

[pscott]

Got it. Provided the "real world" implications of removing this key function, I believe a good middle ground would indeed to return TRUE whenever it does not revert. This way, we can already enforce developers to check the return value of is_valid_signature :)

The potential drawback I could see if a dev writing some special logic in case of invalid transaction, which will never run because the tx will revert. Since this situation is temporary, and that it should be explained in the documentation, I think it is acceptable.