Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Add --memo support to solana command-line #10831

Closed
mvines opened this issue Jun 29, 2020 · 20 comments · Fixed by #16291
Closed

Add --memo support to solana command-line #10831

mvines opened this issue Jun 29, 2020 · 20 comments · Fixed by #16291
Labels
good first issue Good for newcomers
Milestone

Comments

@mvines
Copy link
Contributor

mvines commented Jun 29, 2020

With the SPL Memo program on mainnet-beta (http://explorer.solana.com/account/Memo1UhkJRfHyvLMcVucJwxXeuD728EqVDDwQDxFMNo), we can now add a generic --memo argument to all cli commands that issue transactions to include a memo.

@bji
Copy link
Contributor

bji commented Mar 26, 2021

Hi, I'll take a look at this, if anyone else is working on it or wants to work on it, please let me know!

@CriesofCarrots
Copy link
Contributor

Updated SPL Memo program is here: https://explorer.solana.com/address/MemoSq4gqABAXKb96qnH8TysNcWxMyWCqXgDLGmfcHr
SPL Memo source: https://github.com/solana-labs/solana-program-library/tree/master/memo/program
Let me know if you have any questions!

@bji
Copy link
Contributor

bji commented Mar 30, 2021

In your opinion, does the memo program deserve the same level of support as nonce?

I ask because I am still familiarizing myself with how solana works and I notice that the way nonces are used is very similar to how memos would be used (similar not in application - nonces are very different in purpose than memos - but similar in how they fit into the mechanism of adding an extra instruction to transactions). I am still trying to figure out lots of the details of how transactions are composed and submitted, but I think that very likely memo support would work very similarly to nonce support.

However, nonces appear to be a deeply embedded concept - for example, the sdk knows about them.

Do you envision memo support being that deep? Should the sdk know about them (i.e. adding a "new_with_memo" function very similar to the "new_with_nonce" function of the sdk)? Or should these be inserted into the transaction purely using functions created within the cli source code?

@t-nelson
Copy link
Contributor

In your opinion, does the memo program deserve the same level of support as nonce?

I ask because I am still familiarizing myself with how solana works and I notice that the way nonces are used is very similar to how memos would be used (similar not in application - nonces are very different in purpose than memos - but similar in how they fit into the mechanism of adding an extra instruction to transactions). I am still trying to figure out lots of the details of how transactions are composed and submitted, but I think that very likely memo support would work very similarly to nonce support.

However, nonces appear to be a deeply embedded concept - for example, the sdk knows about them.

Do you envision memo support being that deep? Should the sdk know about them (i.e. adding a "new_with_memo" function very similar to the "new_with_nonce" function of the sdk)? Or should these be inserted into the transaction purely using functions created within the cli source code?

Memo doesn't need to be quite as deep as Durable Nonce. Durable Nonce has a few quirks that justify its specialization. For one, it is implemented as part of the system program and requires special handling in the runtime. Additionally, for a transaction to be flagged Durable Nonce, it must issue a SystemInstruction::AdvanceNonceAccount instruction as the first instruction in a transaction. This is where the Messge::new_with_nonce() helper comes in handy as it ensures that instruction is positioned correctly. Since Memo is part of the SPL, we also want to be careful not to drag that dependency all over the place. So it should be isolated to the cli/ directory in the monorepo.

How I envision this working is a new --memo MEMO_STRING (--with-memo MEMO_STRING ?) optional arg is added to all transaction bearing subcommands and if passed, a Memo instruction is appended to the normal instruction list for that transaction type. Many apologies for the plumbing hell you're about to endure. I have a major refactor planned for CLI that I haven't found time to start yet. The Memo instruction should only need to be added in one place, however due to the current architecture, each subcommand builds, signs and broadcasts its own transaction, so it will need to be added to each 🤕

@CriesofCarrots
Copy link
Contributor

CriesofCarrots commented Mar 30, 2021

Ditto what @t-nelson said. We don't want an sdk dependency on the memo program.

Or should these be inserted into the transaction purely using functions created within the cli source code?

Yes exactly, this ^

@t-nelson
Copy link
Contributor

Or should these be inserted into the transaction purely using functions created within the cli source code?

Yes exactly, this ^

One exception might be a with_memo_arg() helper in clap-utils. I can see that being useful for maintaining consistency with the CLI tools in SPL. This won't require an spl-memo dependency though

@bji
Copy link
Contributor

bji commented Mar 30, 2021

Thank you for your prompt and very helpful answers. And no worries about "plumbing hell", my only lament is that not understanding the overarching design goals (yet!) I don't feel very comfortable making structural changes that would improve this. But in terms of just using the existing patterns, I can totally do that.

@bji
Copy link
Contributor

bji commented Mar 30, 2021

Sorry if this question is a bit dense but ... what would be the point of including a memo with a command line transaction? Is it just to get the memo instructions into the transaction so that they can be looked up later when examining the transaction and thus can be used as a sort of 'tag' on the transaction?

For example would I do: "solana transfer --with-memo="Payment for order 1234567" ..." and then expect later to be able to look at that transfer transaction and be able to then extract the memo data from the instruction stream and associate "Payment for order 1234567" with the transaction?

If so ... does there need to be code in the cli to do that automatically, for example in the 'transaction-history' command?

And if that is so ... when about for example 'solana vote-account'? If the vote-account were created with a transaction that included a memo, would we expect to see that memo emitted in the output of 'solana vote-account'?

@bji
Copy link
Contributor

bji commented Mar 30, 2021

I wonder also, should 'solana block' decode memo data into utf-8 strings as it prints out the decoded instruction data?

@mvines
Copy link
Contributor Author

mvines commented Mar 30, 2021

I wonder also, should 'solana block' decode memo data into utf-8 strings as it prints out the decoded instruction data?

yeah! Here's a block with a memo in it: solana block 63529089

It sadly just prints this:

  Instruction 0
    Program:   Memo1UhkJRfHyvLMcVucJwxXeuD728EqVDDwQDxFMNo (4)
    Data: [49, 45, 108, 56, 51, 104, 45]

@CriesofCarrots
Copy link
Contributor

CriesofCarrots commented Mar 30, 2021

Is it just to get the memo instructions into the transaction so that they can be looked up later when examining the transaction and thus can be used as a sort of 'tag' on the transaction?

Yes, exactly

If so ... does there need to be code in the cli to do that automatically, for example in the 'transaction-history' command?

solana confirm -v and solana block are the main locations in the cli where I would see memo exposed. We have a lot of the plumbing in place for this. Let me take a look to see which bits are missing, and I'll get back to you.

We can't really expose this in account-state queries like solana vote-account without a lot of churn to find out what transaction first created the account. Doesn't seem worth it.

I wonder also, should 'solana block' decode memo data into utf-8 strings as it prints out the decoded instruction data?

Yes, I think that would be nice.

@mvines
Copy link
Contributor Author

mvines commented Mar 30, 2021

solana confirm already knows how to handle the UTF-8 memo correctly, interesting that solana block doesn't -- this probably implies there's some duplicated code in the cli somewhere that can be cleaned up 🧹

$ solana confirm 5EJKHPCtm1nhUZXFXti8N1kR5oKzRFa5F49v3rjh1BtmHXrtY1cewmmyanWGZSntgDDBRrsMbf64RvpEUmKmrhdL -v
RPC URL: http://api.mainnet-beta.solana.com
Default Signer Path: /Users/mvines/.config/solana/id.json
Commitment: confirmed

Transaction executed in slot 63529089:
  Block Time: 2021-02-01T14:18:46-08:00
  Recent Blockhash: 5Q93iCkZZNyhcoXutMGxE1QxeD5EMC148wU3AZbQRzq4
  Signature 0: 5EJKHPCtm1nhUZXFXti8N1kR5oKzRFa5F49v3rjh1BtmHXrtY1cewmmyanWGZSntgDDBRrsMbf64RvpEUmKmrhdL
  Signature 1: 5VFSm54NuB71ZRn4rpLPfBHt2KcqQJsU8veXhBUUq5fLB9J9fEYh9kyooWXR2C48Ng2zbikHTWZWkRUWtYA3vJ2T
  Account 0: srw- tyeraJRJFcHECfTmEGTaCzjcYebfLWETBRniRNuJN12 (fee payer)
  Account 1: sr-- Akpvka7tewujFjceSv9SifvxpY9FqRGqdkD7PiuDZVDV
  Account 2: -r-x MemoSq4gqABAXKb96qnH8TysNcWxMyWCqXgDLGmfcHr
  Instruction 0
    Program:   MemoSq4gqABAXKb96qnH8TysNcWxMyWCqXgDLGmfcHr (2)
    Account 0: Akpvka7tewujFjceSv9SifvxpY9FqRGqdkD7PiuDZVDV (1)
    Account 1: tyeraJRJFcHECfTmEGTaCzjcYebfLWETBRniRNuJN12 (0)
    Data: [240, 159, 166, 150]
  Status: Ok
    Fee: ◎0.00001
    Account 0 balance: ◎98.51226956 -> ◎98.51225956
    Account 1 balance: ◎0
    Account 2 balance: ◎0.52149888
  Log Messages:
    Program MemoSq4gqABAXKb96qnH8TysNcWxMyWCqXgDLGmfcHr invoke [1]
    Program log: Signed by Akpvka7tewujFjceSv9SifvxpY9FqRGqdkD7PiuDZVDV
    Program log: Signed by tyeraJRJFcHECfTmEGTaCzjcYebfLWETBRniRNuJN12
    Program log: Memo (len 4): "🦖"
    Program MemoSq4gqABAXKb96qnH8TysNcWxMyWCqXgDLGmfcHr consumed 46611 of 200000 compute units
    Program MemoSq4gqABAXKb96qnH8TysNcWxMyWCqXgDLGmfcHr success

Finalized

@mvines
Copy link
Contributor Author

mvines commented Mar 30, 2021

oops, correction. that utf shows up in the log but not in the instruction data itself

@bji
Copy link
Contributor

bji commented Mar 30, 2021

OK then sorry to be further dense but ...why does the memo program have any inputs other than data then? If it's just meant to be something that gets injected into the instruction stream as tagging data, why does the program also need to have signers? Is that just so that the memo program is useful as a stand-alone program that at a minimum at least can record a text "message" that can be proven to have been vetted by the signers by virtue of being signed? But for the purposes of embedding a message in another transaction, the extra signers is not necessary ... right? I'm just thinking out loud and want to make sure I understand this.

@CriesofCarrots
Copy link
Contributor

CriesofCarrots commented Mar 30, 2021

OK then sorry to be further dense but ...why does the memo program have any inputs other than data then? If it's just meant to be something that gets injected into the instruction stream as tagging data, why does the program also need to have signers? Is that just so that the memo program is useful as a stand-alone program that at a minimum at least can record a text "message" that can be proven to have been vetted by the signers by virtue of being signed? But for the purposes of embedding a message in another transaction, the extra signers is not necessary ... right? I'm just thinking out loud and want to make sure I understand this.

Yes, you've got that all essentially right. The signing feature in the memo was also targeting a specific use case where a client is tracking transaction logs, and wants to be able to verify the memo and its provenance.

For the this cli application, I don't think that we need to pass any signer addresses to the memo instruction.

@bji
Copy link
Contributor

bji commented Mar 30, 2021

Thank you. I am so new to all of this, a little guidance really helps keep me from going down a lot of thought process dead ends!

@bji
Copy link
Contributor

bji commented Mar 31, 2021

Does this look right?

https://explorer.solana.com/tx/2AFpLAbbWQDuJFp3UN5AcfHKqdPrjAPNroCKaQXpmgVoVh3kPXTLjJXCYMiYhhjPRCobshEbSSRovAsniDWSXd5w?cluster=devnet

@mvines
Copy link
Contributor Author

mvines commented Mar 31, 2021

📈 yep!

bji added a commit to bji/solana that referenced this issue Apr 1, 2021
… that submit

transactions.  Also, improve the block command to show UTF-8 string instead
of integer values for memo program data.
@mergify mergify bot closed this as completed in #16291 Apr 5, 2021
mergify bot pushed a commit that referenced this issue Apr 5, 2021
#16291)

* issue #10831: added --with-memo option to all cli commands that submit
transactions.  Also, improve the block command to show UTF-8 string instead
of integer values for memo program data.

* Fixed tests and changed some syntax according to feedback.

* Use spl_memo id (all versions where applicable) instead of hardcoding id.

* Update Cargo.toml in programs/bpf.

* Update formatting via cargo fmt.

* Update to use spl_memo version 3.0.1, which simplifies package imports
mergify bot pushed a commit that referenced this issue Apr 5, 2021
#16291)

* issue #10831: added --with-memo option to all cli commands that submit
transactions.  Also, improve the block command to show UTF-8 string instead
of integer values for memo program data.

* Fixed tests and changed some syntax according to feedback.

* Use spl_memo id (all versions where applicable) instead of hardcoding id.

* Update Cargo.toml in programs/bpf.

* Update formatting via cargo fmt.

* Update to use spl_memo version 3.0.1, which simplifies package imports

(cherry picked from commit 364af3a)

# Conflicts:
#	cli-output/Cargo.toml
#	cli-output/src/display.rs
#	cli/Cargo.toml
#	cli/tests/transfer.rs
mergify bot pushed a commit that referenced this issue Apr 5, 2021
#16291)

* issue #10831: added --with-memo option to all cli commands that submit
transactions.  Also, improve the block command to show UTF-8 string instead
of integer values for memo program data.

* Fixed tests and changed some syntax according to feedback.

* Use spl_memo id (all versions where applicable) instead of hardcoding id.

* Update Cargo.toml in programs/bpf.

* Update formatting via cargo fmt.

* Update to use spl_memo version 3.0.1, which simplifies package imports

(cherry picked from commit 364af3a)

# Conflicts:
#	cli-output/Cargo.toml
#	cli/Cargo.toml
mergify bot added a commit that referenced this issue Apr 5, 2021
…t (bp #16291) (#16387)

* issue #10831: added --with-memo option to all cli commands that submit (#16291)

* issue #10831: added --with-memo option to all cli commands that submit
transactions.  Also, improve the block command to show UTF-8 string instead
of integer values for memo program data.

* Fixed tests and changed some syntax according to feedback.

* Use spl_memo id (all versions where applicable) instead of hardcoding id.

* Update Cargo.toml in programs/bpf.

* Update formatting via cargo fmt.

* Update to use spl_memo version 3.0.1, which simplifies package imports

(cherry picked from commit 364af3a)

# Conflicts:
#	cli-output/Cargo.toml
#	cli/Cargo.toml

* Fix conflicts

Co-authored-by: bji <bryan@ischo.com>
Co-authored-by: Tyera Eulberg <tyera@solana.com>
@fabiooshiro
Copy link

Can I send the memo using web3.js transaction?

@CriesofCarrots
Copy link
Contributor

@fabiooshiro , yes. There will be a npm package when this PR is complete: solana-labs/solana-program-library#2583
In the meantime, you can use the code from memo/ts/src/index.ts directly

@solana-labs solana-labs locked and limited conversation to collaborators Nov 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants