Open enitrat opened 4 months ago
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:
am I missing anything important? are you concerned about some specific risk?
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
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).
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?
π 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.