feature: add memo field to stx-transfer clarity function

[pavitthrap]

Description

Added an optional memo field to the stx-transfer? clarity function (by optional I mean you can choose to fully omit this argument when calling the function).

Fixes #2391.

Also fixes #2485, which is a small DEBUG logging change.

Notes

• I decided to make the memo input buff(34) instead of option(buff(34)). The reason for this is because memo was provided as a string directly through the struct TransferStxOp in src/chainstate/stacks/db/blocks.rs, so I wanted to keep things consistent with that. Moreover, for backwards compatibility, the memo field is completely optional to even provide, so making the type explicitly an option felt a little unnecessary. Let me know if anyone has any opinions on this.
• In special_stx_transfer, I first check if there are 4 arguments. If there aren’t I then check if there are 3 arguments, which returns an error directly if there are not 3 arguments. Should I instead return an error message that the stx-transfer? function expects 4 arguments, instead of letting the check_argument_count(3, args)? return its error directly? Could be a source of confusion.
• For some of the calls to stx-transfer? in the repo (such as in test functions), I passed in the memo field, but not for all the calls. I did this because the function can now take in either 3 or 4 arguments so I thought it would be good to have instances of both in our tests. Let me know if anyone thinks I should pass in the additional memo parameter to all the tests throughout the repo.

Type of Change

• New feature
• Bug fix
• API reference/documentation update
• Other

Does this introduce a breaking change?

Yes

Are documentation updates required?

Yes, this adds an additional field to stx-transfer?. Refer to change in src/vm/docs/mod.rs.

Testing information

Run cargo test, there are various tests for this function in the code base.

For the debug logging change(issue #2485), I ran a stacks node locally and checked for a change in the DEBUG output for all_burns. Example of new output:
DEBG [1614734509.490227] [src/chainstate/burn/distribution.rs:289] [chains-coordinator] Burn sample, txid: 72389d8e5e512d72d4691db8ee98ada89559aeefe3bc72cb5d2f7e0144bb6997, most_recent_burn: 33332, median_burn: 1, all_burns: [33332, 1, 1, 33332, 1, 1]

[zone117x]

I think we'd also need to a memo property to the stx_transfer_event data, otherwise I don't think the API and tooling will able to easily use this new feature
https://github.com/blockstack/stacks-blockchain/blob/fe81b98d81df289686140941a6d65ac49f2c63f4/src/chainstate/stacks/events.rs#L93

[pavitthrap]

I think we'd also need to a memo property to the stx_transfer_event data, otherwise I don't think the API and tooling will able to easily use this new feature

https://github.com/blockstack/stacks-blockchain/blob/fe81b98d81df289686140941a6d65ac49f2c63f4/src/chainstate/stacks/events.rs#L93

I added memo as a field in STXTransferEventData, so this should be covered.

[kantai]

These changes look great to me, thanks @pavitthrap, just had a couple of small nits.

[zone117x]

This is great @pavitthrap!
Code changes LGTM. I think the changelog or similar needs updated with this change. That will be helpful for the API, for example, when implementing Stacks 2.1 upgrades.
Maybe a "Stacks 2.1 upgrade notes" log or something similar?

[reedrosenbluth]

Excellent work! One small comment but otherwise LGTM!

[reedrosenbluth]

I'd add to the description here that memo can optionally be omitted

[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.