missing any form of indication that SD wallet sent a memo #84

Closed
opened 2 years ago by jahway603 · 10 comments
Collaborator

After sending a transaction from SD 1.3.0 to another client, with text in the memo field, the receiving client is able to see the memo successfully but the sending client shows nothing that a memo was ever sent. Is this design feature or a bug?

After sending a transaction from SD 1.3.0 to another client, with text in the memo field, the receiving client is able to see the memo successfully but the sending client shows nothing that a memo was ever sent. Is this design feature or a bug?
Owner

@jahway603 I consider this a bug. This is related to the advanced setting in the "senttxstore.dat". If "save sent tx" is not set, no tx data sent from a zaddr will be shown. When we inherited this code originally, there was no RPC to show data sent from a specific zaddr, now we have z_listsentbyaddress. Additionally, if you sent ztx's from hush-cli, those never get stored in senttxstore.dat and so they will never show up at all in the SD GUI, which I find confusing and sucky. How we render ztx data in our transaction tab needs to be updated.

@jahway603 I consider this a bug. This is related to the advanced setting in the "senttxstore.dat". If "save sent tx" is not set, no tx data sent from a zaddr will be shown. When we inherited this code originally, there was no RPC to show data sent from a specific zaddr, now we have `z_listsentbyaddress`. Additionally, if you sent ztx's from `hush-cli`, those never get stored in `senttxstore.dat` and so they will never show up at all in the SD GUI, which I find confusing and sucky. How we render ztx data in our transaction tab needs to be updated.
Collaborator

I can confirm this issue exists when sending from SD. The tx shows in list but no memo icon displays in sending client and double clicking the tx shows no memo either.

I can look into seeing how SD GUI is currently pulling and rendering this from senttxstore.dat. Can probably fix without changing more stuff to support txs sent from hush-cli.

senttxstore.dat stores data like this for reference. Probably just missing memo support when reading it.

[
    {
        "address": "zs1reciever",
        "amount": -0.0001,
        "datetime": 1233456789,
        "fee": -0.0001,
        "from": "zs1sender",
        "memo": "Y0!\nReply to:\nzs1sender",
        "txid": "txid",
        "type": "sent"
    }
]
I can confirm this issue exists when sending from SD. The tx shows in list but no memo icon displays in sending client and double clicking the tx shows no memo either. I can look into seeing how SD GUI is currently pulling and rendering this from senttxstore.dat. Can probably fix without changing more stuff to support txs sent from hush-cli. senttxstore.dat stores data like this for reference. Probably just missing memo support when reading it. ``` [ { "address": "zs1reciever", "amount": -0.0001, "datetime": 1233456789, "fee": -0.0001, "from": "zs1sender", "memo": "Y0!\nReply to:\nzs1sender", "txid": "txid", "type": "sent" } ] ```
Owner

@fekt I feel like the memo of sent ztx's is not stored in senttxstore.dat, because that file is not encrypted, and storing memos in plaintext on disk is not a great idea. The data is buried somewhere in wallet.dat, also not encrypted (ugh) but might not have an RPC to get the memo for sent ztx's, or maybe can be pulled via a new RPC, like getalldata, which SD does not use yet

@fekt I feel like the memo of sent ztx's is not stored in senttxstore.dat, because that file is not encrypted, and storing memos in plaintext on disk is not a great idea. The data is buried somewhere in wallet.dat, also not encrypted (ugh) but might not have an RPC to get the memo for sent ztx's, or maybe can be pulled via a new RPC, like `getalldata`, which SD does not use yet
Collaborator

@fekt I feel like the memo of sent ztx's is not stored in senttxstore.dat

The memo and all data is stored in plaintext like JSON above. SentTxStore just doesn't set the memo from the data in the file. It looks like an easy fix here:
https://git.hush.is/hush/SilentDragon/src/branch/dev/src/senttxstore.cpp#L45

You are right it's bad idea for all this to be unencrypted. I saw this issue: #65

Not sure on best way to handle it but using any new RPCs instead of writing to disk would probably be easier than adding file encryption. Since the data is already there, I can try adding quick fix to existing functionality.

> @fekt I feel like the memo of sent ztx's is not stored in senttxstore.dat The memo and all data is stored in plaintext like JSON above. SentTxStore just doesn't set the memo from the data in the file. It looks like an easy fix here: https://git.hush.is/hush/SilentDragon/src/branch/dev/src/senttxstore.cpp#L45 You are right it's bad idea for all this to be unencrypted. I saw this issue: https://git.hush.is/hush/SilentDragon/issues/65 Not sure on best way to handle it but using any new RPCs instead of writing to disk would probably be easier than adding file encryption. Since the data is already there, I can try adding quick fix to existing functionality.
Collaborator

Easy fix works just setting the memo. Renders icon in GUI and double click shows memo. ab17a1012e

Easy fix works just setting the memo. Renders icon in GUI and double click shows memo. https://git.hush.is/hush/SilentDragon/commit/ab17a1012ebe8e5aab078934a4e7cabec0560893
Owner

@fekt yeah, I have vague memories that maybe ZecWallet code originally did store the memo field unencrypted on disk, and I might have changed that to store the empty string, because it seemed like a bad idea. Originally, SDL was the same, but then Denio and I worked hard to encrypted everything on disk, so now SDL does things in a better way than SD.

I will look into what RPC can give us the data or make a new RPC that can do it.

@fekt yeah, I have vague memories that maybe ZecWallet code originally did store the memo field unencrypted on disk, and I might have changed that to store the empty string, because it seemed like a bad idea. Originally, SDL was the same, but then Denio and I worked hard to encrypted everything on disk, so now SDL does things in a better way than SD. I will look into what RPC can give us the data or make a new RPC that can do it.
Collaborator

@duke Clarifying that empty string I modified is what made it not display in SD and is when it's reading the file. The memo and everything gets stored unencrypted starting here in addToSentTx: ab17a1012e/src/senttxstore.cpp (L101)

@duke Clarifying that empty string I modified is what made it not display in SD and is when it's reading the file. The memo and everything gets stored unencrypted starting here in `addToSentTx`: https://git.hush.is/hush/SilentDragon/src/commit/ab17a1012ebe8e5aab078934a4e7cabec0560893/src/senttxstore.cpp#L101
Owner

@fekt ok, your fix is good and an actual bugfix, this old commit did it 💩 : d37cf339c9

And now to make things better, I think it's really valuable to encrypt senttxstore.dat. The best way will be to make a new encrypted .dat file with a different name, such as sd.dat, and all future encrypted wallet metadata will go in there, such as when we migrate addresslabels.dat and/or user config settings.

@fekt ok, your fix is good and an actual bugfix, this old commit did it :poop: : https://git.hush.is/hush/SilentDragon/commit/d37cf339c9d2c7a971a6e768208b8c1860dbbed8 And now to make things better, I think it's really valuable to encrypt senttxstore.dat. The best way will be to make a new encrypted .dat file with a different name, such as sd.dat, and all future encrypted wallet metadata will go in there, such as when we migrate addresslabels.dat and/or user config settings.
Collaborator

I think it's really valuable to encrypt senttxstore.dat.

Agreed. I'm just not sure how to go about it and would probably need an option or something during setup to add PIN or password like SDL.

>I think it's really valuable to encrypt senttxstore.dat. Agreed. I'm just not sure how to go about it and would probably need an option or something during setup to add PIN or password like SDL.
Owner

@fekt you are right, it's a bit involved, but if we work together on it, I think it's feasible and will teach you a lot more about the internals.

Going to call this issue done and we can focus on #96 to encrypt data on disk.

@fekt you are right, it's a bit involved, but if we work together on it, I think it's feasible and will teach you a lot more about the internals. Going to call this issue done and we can focus on #96 to encrypt data on disk.
duke closed this issue 2 years ago
Sign in to join this conversation.
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.