read-only functions can not call trait functions even if the implementation uses a read-only function

[psq]

The following code

(define-read-only (symbol)
  (contract-call? 'ST3J2GVMMM2R07ZFBJDWTYEYAR8FZH5WKDTFJ9AHA.swapr symbol 'ST3J2GVMMM2R07ZFBJDWTYEYAR8FZH5WKDTFJ9AHA.plaid-token 'ST3J2GVMMM2R07ZFBJDWTYEYAR8FZH5WKDTFJ9AHA.stx-token)
)

generates an error expecting read-only statements, detected a writing operation. (i.e. WriteAttemptedInReadOnly) when deploying, even if contract-call? is calling an other define-read-only function.

This should be allowed.

Fix may be related to what is needed for #1641, and the 2 issues prevent using the RPC read-only call to call any function using contract-call (if you make the function define public then #1641 prevents you as well.

[psq]

This is actually a bit more subtle and specific to traits. Traits definition do not specify whether a function is read-only or not, so read-only detection can only be done either at run time, or call time, not deploy time. If calling a read-only function from an other contract, this works fine.

More specifically, here's a full example that fails with the same error:

(define-trait fun-trait ( (fun1 () (response uint uint)) ))

(impl-trait .trait.fun-trait) (define-read-only (fun1)
  (ok u1))

(use-trait fun-trait .trait.fun-trait)
(define-read-only (fun2 (fun <fun-trait>))
  (contract-call? fun fun1))

updated the title to reflect the actual issue.

One possibility is to relax the implementation of type_check, and then check the contract passed at runtime uses a read-only function before accepting the transaction.

An other possibility would be to change the trait definition to be able to specify some functions should only be defined as read-only.

Fixing #1641 would also allow calling such functions if you give up on using define-read-only.

I tend to favor the first option. Thoughts?

cc @kantai @lgalabru @jcnelson

[friedger]

A third option would be to introduce contract-call-read-only? or so, and then check at runtime. I think this makes it more obvious for developers (and users) to understand that the state does not change.

[jcnelson]

My personal philosophy on this is to "never put off until runtime that which can be done at compile-time" and to "make invalid states unrepresentable." I'm in favor of updating define-trait so the trait definition can require the implementation to be read-only. Then, a read-only function can call a trait implementation without fear of runtime errors. Also, just as importantly, the developer can get immediate feedback as to whether or not a supplied trait implementation is valid with respect to read-only-ness.

[psq]

yes, I can see your point.

So here's my proposal (using the trait swapr uses for tokens, so far):

(define-trait src20-trait
  (
    ;; Transfer from the caller to a new principal
    (transfer (principal uint) (response bool uint))

    ;; the human readable name of the token
    (name () (response (string-ascii 32) uint) (read-only))

    ;; the ticker symbol, or empty if none
    (symbol () (response (string-ascii 32) uint) (read-only))

    ;; the number of decimals used, e.g. 6 would mean 1_000_000 represents 1 token
    (decimals () (response uint uint) (read-only))

    ;; the balance of the passed principal
    (balance-of (principal) (response uint uint) (read-only))

    ;; the current total supply (which does not need to be a constant)
    (total-supply () (response uint uint) (read-only))
  )
)

I'm using a list, so potentially you can have other attributes, such as optional maybe. And having this at the end of the definition looks better to me.

A function with the read-only attribute would need to be implemented with define-read-only to be a valid trait implementation.

[kantai]

This proposal looks good to me, @psq -- what do you think @lgalabru ?

[lgalabru]

Looks good to me as well, thank you @psq!

[psq]

great, will get a PR ready

[njordhov]

I'm using a list, so potentially you can have other attributes, such as optional maybe. And having this at the end of the definition looks better to me.

@psq Consider using a record structure for properties such as declaring trait functions to be read-only. Like:

(balance-of (principal) (response uint uint) {read-only: true})

[jcnelson]

Temporarily assigning to @obycode for now, so this has an owner. Please re-assign as you see fit.