Open obycode opened 2 years ago
For consideration, there are forward compatibility benefits of going the other direction, allowing local bindings to shadow globals. For example, say the binding instead is to a symbol attempt
as below. Now, if attempt
becomes a reserved keyword in a future version of Clarity, the contract will fail when redeployed.
(define-public (call-this (fail bool))
(let ((attempt (try-me fail)))
(ok attempt)))
That's a good point @njordhov, and would make a lot of sense for other languages, but for Clarity, I think we're looking to maximize readability and minimize the potential for misleading code, so I'd argue that the forward incompatibility is a reasonable price to pay to disallow potentially confusing uses of reserved keywords.
Forward compatibility is particularly relevant for Clarity, as contracts deployed on-chain cannot be updated.
There is a considerable trade-off in sacrificing forward compatibility for the sake of disallowing potentially confusing uses of reserved keywords. Without such forward compatibility, every new function added to the language is a breaking change, requiring special handling of existing contracts. The VM will increase in complexity to accommodate incompatible contracts (see issue https://github.com/stacks-network/stacks-blockchain/issues/3131).
Yet little is gained as disallowing some symbols doesn't hinder developers from using similarly confusing symbols that resemble built-in functions. Advising developers on problematic name choices is better covered by a linter like clarinet check, leaving the VM to focus on correctly executing contracts.
For your particular example, the confusion can be resolved by using a more conventional formatting of the code, making it apparent that try
is not called as a function:
(define-public (call-this (fail bool))
(let ((try (try-me fail)))
(ok true)))
For 2.1, we can make it so that binding to reserved keywords will result in a CheckError
, thereby preventing such transactions from getting mined.
Linking #2696 as well!
Are there actually any keywords in Clarity that strictly have to be reserved and not allowed in local bindings?
block-height
, tx-sender
, contract-caller
, true
, false
, none
, some
, burn-block-height
, stx-liquid-supply
- these are considered as illegal during execution, but not during analysis (contract deployment).
Some (I think all top level) function names aren't considered as illegal at all ie. define-data-var
, define-public
.
Most if not all function names are considered as illegal only during execution.
In short I would say that every single keyword that is part of language syntax should be considered as reserved.
In Clarity, keywords don't have to be in the language syntax, thus strictly don't have to be reserved.
Clarity has a simple syntax in the form of S-expressions where the parsing is independent of the keywords. In other languages, keywords are reserved because they are part of the grammar, with the keywords determining the parsing.
I agree, @njordhov, that they are not required to be reserved, but again, to avoid confusing (innocent or malicious) contracts, I agree with @LNow above and prefer to reserve all of the keywords.
@obycode, the argument posted by @LNow does not support your position. @LNow argued that every single keyword that is part of language syntax should be considered as reserved but we already agree that no keywords have to be part of the language syntax. There are better ways to solve the concern of potentially confusing local bindings without the sacrifices and complications associated with reserving keywords.
I'm inclined to agree with @njordhov on this topic on a high level (without involving my thinking in the implementation details)...
A couple of spontaneous thoughts:
@
symbol. This syntax makes it clear to someone reading the contract that it's something provided by the author and not the language. (what a coincidence that Clarity currently doesn't use @
π
)I think it's important to remember that even if one of the key goals of Clarity is to be "clear", the average user isn't going to read the contracts. I think that maybe focusing on auditing tooling (e.g. npm audit
?) would have a broader use/greater impact than whether to-or-not-to reserve keywords. A site that lets the average lamen check a contract and get some sort of "trust score" (maybe also integrated into wallets?) would go a looonngg way to increase trust in the ecosystem. It's not the developers that need to feel safe after all, it's the end users...
/ my 2 cents
for Clarity, I think we're looking to maximize readability and minimize the potential for misleading code, so I'd argue that the forward incompatibility is a reasonable price to pay to disallow potentially confusing uses of reserved keywords.
These desired benefits of reserving keywords are moot with Stacks 2.1. The new Clarity version scheme defeats readability/security gains of reserving names: A black hat can simply upload a confusing contract as Clarity 1 while pretending to call native Clarity 2 functions, redefining them in the contract to do something else than expected.
Good point. I agree, that's a good argument for the in-contract version indicator.
that's a good argument for the in-contract version indicator.
This π π
SIP-015 allows the publish transaction variant to include a version byte to explicitly set the VM version to use. If not provided, the default version for the epoch (e.g. Clarity 2 for Stacks 2.1) will be used.
SIP-015 allows the publish transaction variant to include a version byte to explicitly set the VM version to use. If not provided, the default version for the epoch (e.g. Clarity 2 for Stacks 2.1) will be used.
I think that a version metadata tag at the top of a contract (like docker compose) would be more of a help to tooling (IDE's, clarinet, etc.) than anything, but also make it clear in public contracts on e.g. github which Clarity version the contract was written for (removing the need to explicitly specify this in the publish trx).
It also fixes the Clarity version directly to the contract, which would be beneficial in the case where new Clarity versions change/remove functions, where automatically using the current epoch version might fail (if forgotten to explicitly set on the pub trx).
Of course, it's not strictly needed, would just make things more explicit π€·ββοΈ
I think that a version metadata tag at the top of a contract (like docker compose) would be more of a help to tooling (IDE's, clarinet, etc.) than anything, but also make it clear in public contracts on e.g. github which Clarity version the contract was written for (removing the need to explicitly specify this in the publish trx).
So can this:
;; This is a Clarity 2 contract.
It also fixes the Clarity version directly to the contract, which would be beneficial in the case where new Clarity versions change/remove functions, where automatically using the current epoch version might fail (if forgotten to explicitly set on the pub trx).
Unless you're packaging up your code directly into a transaction (in which case, you should know better), there's no reason the contract-publish tooling can't just do this for you. In fact, the tooling could require a pragma of some kind (e.g. specified in a code comment) in order to determine which transaction variant to create.
The reasons I'm against adding a Clarity function to encode the version are:
(clarity-version 2)
, (clarity-version \n 2)
, etc.)Anyway, this is definitely not happening in Stacks 2.1, and until we run out of values to put in that byte, I think we can close this issue.
@obycode Regarding the original issue at hand -- illegal use of reserved keywords -- is this fixed by the new parser?
Adding a note here following discussion at the engineering weekly meeting -- this is intentionally not changed in 2.1's parser or type-checker changes, since there was still open discussion about it and it is not explicitly clarified in the SIPs. That being said, Clarinet is intentionally opinionated, so we could catch these situations in Clarinet and report errors there. I have reopened the related issue in the Clarinet repo.
Assigning this to @obycode. Please feel free to close or re-assign. This won't ship in 2.1, but it could ship with sBTC.
Copied from hirosystems/clarinet#418 reported by @FriendsFerdinand. I opened a PR to fix this in clarinet, but a similar change should likely be made here, for 2.1, so that this type of problem will cause an error when deploying the contract instead of only reporting a runtime error when it is called.
Original issue:
Consider the following contract where I use
try!
as a variable name:This passes a clarinet check, but when executed on clarinet console returns a RuntimeError:
If you use an asserts! instead, it will not pass clarinet check, naming a syntax error. If I write:
(define-data-var try! uint u0)
, then it will return a Runtime errorNameAlreadyUsed
after running clarinet check.Conclusion
I think running clarinet check should be able to detect this kind of error before trying to run the contract code on clarinet console/test.