call_contract_syscall errors cannot be caught and handled

[enitrat]

📝 Details

https://github.com/OpenZeppelin/cairo-contracts/blob/main/src/utils.cairo#L14-L26

While the language lets us catch errors in call_contract_syscall and local testing frameworks like cairo-test or snfoundry will let us do this, the StarknetOS currently cannot catch errors during syscall - and any transaction where a syscall fails will fail. As such, I don't think this should be used in the openzeppelin libs.

The indication is in the readme but I find it misleading to keep it in the source code as it can be misleading to some people.

[martriay]

hey there @enitrat thanks for bringing this up! our overall feeling regarding this is to leave stuff as it is.

A few thoughts supporting this idea are:

• we want to be ready for when this is finally supported in the OS. we expect all contracts to be automatically supported once the feature is enabled, hugely advancing the interface migration and overall interoperability of the network
• I see removing it as a step back in how we're addressing the overall issue, and would rather see Starknet taking one step ahead. let's exert the pressure forward not backwards
• in practical terms, leaving it as it is would just fail the same, contract behavior is not very different
• to your point though you’re right it may confuse some people and help set false expectations, but I also believe anyone writing smart contracts ought to carefully read the docs of what they're using and this is literally the first thing you see. Dual dispatchers more specifically are not something you use inadvertently
• even if there's some confusion, this will just fail as if they wouldn't have used the module, with the exception that they will automatically work once StarknetOS implements it

am I missing anything important? are you concerned about some specific risk?

[enitrat]

am I missing anything important? are you concerned about some specific risk?

The reason why I opened this issue initially was that someone reached out to me asking questions about this fallback mechanism; I was initially surprised to see it was used in the released contracts. There's no actual "risk", except the one of people deploying contracts that will pass tests, but not work in a real environment - and I have seen in the past projects using this fallback mechanism thinking that it would work in prod.

we want to be ready for when this is finally supported in the OS

I understand this, but I think this is still quite far from now

and would rather see Starknet taking one step ahead. let's exert the pressure forward not backwards

Me too 😆 but from what I remember it's not planned before 0.15

[0xEniotna]

Hey, I'm auditing a project right that is using this pattern. Every dev's first reflex is to copy OZ's code to build stuff. While it is usually the BEST thing to do, if there is a bug in one of OZ's implementation (which basically never happens), this bug will appear in every other codebases....
Fortunately this one isnt critical but still, it could lead to unexpected behaviours (calls that always fail etc).

[martriay]

No bug at all! This works as intended and documented, and it will start working properly whenever the feature is implemented onchain. We've designed our contracts to be ready and compatible with such event.

Anyone using the dual dispatchers should read the docs as this is not a feature you use inadvertently, and anybody copypasting well... might eventually find out it's not the best practice for building smart contracts.

I still believe we should not remove the feature to help copypasters at the expense of less interoperability down the road, since the worst case scenario seems to be just a bit of confusion among the ones that didn't read the docs. Do you think I'm missing something?