fix: handle empty type_args in define-trait definition

[hugocaillard]

Safety check in parse_trait_type_repr

[hugocaillard]

@jcnelson I think both are possible. We could probably add a check in clarinet, but on there other, I think it's fine to have this code path being safe as well

[jcnelson]

@hugocaillard The runtime crash that this patch addresses does not manifest in the Stacks blockchain. It only exists in Clarinet. Therefore, the fix for it should be in Clarinet.

[kantai]

@hugocaillard The runtime crash that this patch addresses does not manifest in the Stacks blockchain. It only exists in Clarinet. Therefore, the fix for it should be in Clarinet.

I don't agree with the conclusion -- while its true that this check is currently unnecessary in this repo, the function has an implicit pre-condition that the caller has performed this check. Ultimately, having this hanging around in the codebase is dangerous, because a future use of this function could forget to perform the pre-check. It's also perfectly safe to add this check to codebase right now for exactly the reasoning you mention: the stacks-node already performs the check before calling this function, so the check failure is unreachable from there anyways.

[jcnelson]

The issue in Clarity's trait definition parser pertaining to this crash has less to do with the fact that it's performing a slice access, and more to do with the fact that the safety of the code path in the Stacks blockchain depends on the input first being validated by a call to DefineFunctionsParsed::try_parse(). At a minimum, the problem in the code here is that this pre-condition is not documented in parse_trait_type_repr. However, my ideal outcome is that parser code like DefineFunctionsParsed::try_parse() simply returns the first argument (e.g. function name) separately from its arguments, thereby obviating the need for a direct slice in code that consumes this output. This isn't specific to the trait parser; the whole of the Clarity codebase would benefit from this structural change.

[kantai]

However, my ideal outcome is that parser code like DefineFunctionsParsed::try_parse() simply returns the first argument (e.g. function name) separately from its arguments, thereby obviating the need for a direct slice in code that consumes this output. This isn't specific to the trait parser; the whole of the Clarity codebase would benefit from this structural change.

This would be a larger refactor and require much more testing. I agree that it would be a worthwhile refactor, but I don't want to delay the easy removal of a trap with the hope of a later larger refactoring.

[jcnelson]

@kantai I'm not opposed to patching the Stacks blockchain, for the reasons you point out (I posted my above comment before I saw yours).

What I'm opposed to is merging this before Clarinet is patched, or instead of patching Clarinet.

EDIT: Just saw your second comment. Yes, I agree that it's worthwhile to merge this. But I don't like the idea of Clarinet passing the buck to us to fix problems that they have the responsibility to address.

[obycode]

But I don't like the idea of Clarinet passing the buck to us to fix problems that they have the responsibility to address.

I'd argue that this was a case of something potentially dangerous in the stacks-core code, that happened to be caught by Clarinet. This usage could be fixed in Clarinet, but if we want to have the fix in stacks-core any way, then there seems to be no need to also change the way this function is used in Clarinet.

[blockstack-devops]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.