SIP010 fix: memo clarification

[MarvinJanssen]

An amendment to clarify how to deal with memos in transfer. This is based on various discussions on Discord.

The recommendation was to follow along with the stx-bulk-transfer tool https://github.com/blockstack/send-many-stx-cli. The relevant code:

(define-public (send-stx-with-memo (ustx uint) (to principal) (memo (buff 34)))
 (let ((transfer-ok (try! (stx-transfer? ustx tx-sender to))))
   (print memo)
   (ok transfer-ok)))

From which the following was deduced:

1. The memo print event should come after ft-transfer?.
2. It should be unwrapped before being printed.
3. print should always be called.

It was pointed out that the order is important in order to always be able to rely on event index 1.

Those part of the discussion would have noticed that I was already not a fan of having the optional memo be part of transfer in lieu of introducing a transfer-memo, like the upcoming STX counterpart stx-transfer-memo?. Nonetheless, since SIP010 is ratified we can at least clarify the language on the intended use of the memo parameter, as contracts in the wild are already handling it differently. (When to print, unwrap yes/no?) Some even completely disregard the memo field. Example:

https://github.com/wrappedfi/tokensoft_token_stacks/blob/d3288069bbb0c4a5c45946eb363e0a9ace359222/contracts/tokensoft-token.clar#L56

Some questions & thoughts:

1. Regarding the ordering, what if a DeFi contract emits custom print events before calling into transfer with a memo? I think putting in language that implies reliance on event order is a mistake.
2. The recommendation to make the behaviour on par with the send-many tool makes the optional superfluous. The SIP might as well have just defined memo as (buff 34). Should one only print if (is-some memo)? Consider:

   (print (default-to 0x memo))
   ;; or
   (match memo to-print (print to-print) 0x)

cc @friedger @whoabuddy @LNow

[friedger]

Cc @hstove

[jcnelson]

Hey @MarvinJanssen,

The SIP process does not currently have an amendment procedure, and this SIP did not create a provision for one. Once a SIP is ratified, it can only be superseded. This is particularly true here, because SIP-010 specifies a contract trait definition that is already deployed (and thus cannot be changed).

Without changing the SIP process, the way forward would be to submit a new copy of the text of SIP-010 but with a different SIP number. If ratified, it would replace SIP-010 as the de facto fungible token interface standard, and the activation status of SIP 010 would be updated from Ratified to Replaced.

That said, I'm also open to changing the SIP process to create an amendment procedure. However, this would require a governance-track SIP to describe the procedure, and would need to be ratified before this change can be made. This would be a one-off task that, once complete, would make it possible to modify already-ratified SIPs without giving them a new SIP number.

[MarvinJanssen]

@jcnelson I realise that. The PR does not suggest to change the trait, merely to clarify the language surrounding the memo. For me, it fell within the realm of "fixing typos", although others might disagree.

[hstove]

I would like to see an analysis that looks at the most cost effective way to implement this guidance. IMO we should ensure that, if memo is none, we do not waste any further cost.

[hstove]

I also personally agree with Marvin that this is just a useful clarification - not a material change in the trait or any part of the spec. But I also understand the desire to stick to SIP processes.

[MarvinJanssen]

@hstove I agree. Also memo being optional implies that no print should happen if it is none. (Considering (some 0x) vs none.) But that would make the behaviour different from the send-many tool.

I think I am in favour of no print if memo is none; i.e. (match memo to-print (print to-print) 0x).

[jcnelson]

Ah, I see. As long as this is only a clarification, then it can be merged straight away (assuming @hstove signs off on it).

[MarvinJanssen]

If @hstove signs off on it then we might still want to spend some time on the questions in the OP. (Especially with people who work on implementing / supporting SIP010 tokens as they are the ones impacted by how and if the memo is emitted.)

[LNow]

@hstove, right now simple (print memo) is cheaper than any other option.

[MarvinJanssen]

To note, that will print a (some 0x1234..) which is a departure from the send-many tool which prints the buff directly. (Not saying it's a bad thing per se, just pointing it out.)

[LNow]

I know. Personally I think we should use (match memo to-print (print to-print) 0x)

And to ensure that ft-transfer? and print are executed in correct order, we should also think about providing simple unit test suite, that developers could use to verify if their tokens are build according to the standard or not.

[MarvinJanssen]

@hstove any thoughts? 😁

[MarvinJanssen]

I decided to update the the text based what @LNow and I think is right but still waiting for input. We should probably reach a resolution soon, whether it is to merge or reject this PR.

[jcnelson]

I'm fine with this change. You ready to merge it?

[MarvinJanssen]

Yep. I guess we are still waiting for a verdict from @hstove.

[MarvinJanssen]

@jcnelson should we go ahead with this? Seems people are generally in favour but the PR is getting stale.

[jcnelson]

Yeah sounds good to me