Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add note about get_transfers/get_transfer_by_txid destinations being undefined, make it more consistent #2306

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

recanman
Copy link

No description provided.

Copy link

netlify bot commented Jun 16, 2024

Deploy Preview for barolo-time-757cf9 ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 8929891
🔍 Latest deploy log https://app.netlify.com/sites/barolo-time-757cf9/deploys/6690ca5e4064a20008101de7
😎 Deploy Preview https://deploy-preview-2306--barolo-time-757cf9.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@recanman recanman changed the title add note about get_transfers/get_transfer_by_txid destinations being undefined add note about get_transfers/get_transfer_by_txid destinations being undefined, make it more consistent Jun 16, 2024
@HardenedSteel
Copy link
Contributor

squash?

@recanman
Copy link
Author

@HardenedSteel squashed

@plowsof
Copy link
Collaborator

plowsof commented Jul 12, 2024

would it not be better to just add a "Note: ..." next to the destinations* param? the scary warning for unlock time was an exception, unless this is of the same severity if overlooked?

@@ -1984,6 +1985,7 @@ $ curl http://127.0.0.1:18082/json_rpc -d '{"jsonrpc":"2.0","id":"0","method":"g
Show information about a transfer to/from this address.

<p style="color:red;"><b>WARNING</b> Verify that the transfer has a sane <code>unlock_time</code> otherwise the funds might be inaccessible.</p>
<p style="color:red;"><b>WARNING</b> <code>destinations</code>, only available for outgoing transactions, will be <b>UNDEFINED</b> if not in the wallet cache.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of a nitpick, but it would be nice if this message matched the one above.

@jeffro256
Copy link
Contributor

would it not be better to just add a "Note: ..." next to the destinations* param? the scary warning for unlock time was an exception, unless this is of the same severity if overlooked?

I think it's worthy of a warning, since you can permanently lose accounting information, which I remember caused at least one person financial damage since they couldn't provide a valid tx spend proof to their exchange.

@plowsof
Copy link
Collaborator

plowsof commented Sep 2, 2024

with the new docs site we can play around with warnings.

would it not be better to just add a "Note: ..." next to the destinations* param? the scary warning for unlock time was an exception, unless this is of the same severity if overlooked?

I think it's worthy of a warning, since you can permanently lose accounting information, which I remember caused at least one person financial damage since they couldn't provide a valid tx spend proof to their exchange.

the possible repercussions of deleting the wallet cache / re-syncing a wallet would fill the entire doc with red warnings. so i would NACK this PR as is. This would seem better fitting as a Note about working with wallets / monero-wallet-rpc. The new docs site can facilitate this better i feel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants