hirosystems / clarinet

Write, test and deploy high-quality smart contracts to the Stacks blockchain and Bitcoin.
https://hiro.so/clarinet
GNU General Public License v3.0
307 stars 141 forks source link

Using `try!` as variable name in `let` should not pass a clarinet check #418

Open FriendsFerdinand opened 2 years ago

FriendsFerdinand commented 2 years ago

Description

Consider the following contract where I use try! as a variable name:

(define-public (call-this (fail bool))
  (let (
    (try! (try-me fail))
  )
    (ok true)
  )
)

(define-public (try-me (fail bool))
  (begin
    (asserts! fail (err u999))
    (ok true)
  )
)

This passes a clarinet check, but when executed on clarinet console returns a RuntimeError: Runtime Error: Runtime error while interpreting ST1PQHQKV0RJXZFY1DGX8MNSNYVE3VGZJSRTPGZGM.contract-7: Unchecked(NameAlreadyUsed("try!"))

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 error NameAlreadyUsed 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.

obycode commented 2 years ago

Thanks for reporting this @FriendsFerdinand.

I see that the type-checker does not include the built-in reserved keywords when it does its checks, but during execution, this is checked. We should be able to just add this check into ContractContext::check_name_used.

obycode commented 2 years ago

I submitted a PR to fix this in clarinet, but this should also be fixed in the clarity lib in stacks-blockchain. I will open a corresponding issue there.

obycode commented 2 years ago

We should also ensure that the unexpected behavior from https://github.com/stacks-network/stacks-blockchain/issues/2696 is unaffected by this change.

lgalabru commented 2 years ago

This will be fixed at the canonical vm level, will close this issue. Feel free to reopen if you see another path forward.

obycode commented 1 year ago

This change is not included in 2.1. For the time being, let's report this as an error in Clarinet, and then push to get this included in the next network fork.

smcclellan commented 1 year ago

@hugocaillard Has this been resolved upstream?