slimcoin-project / pacli

Simple CLI PeerAssets client (extended version).
GNU General Public License v3.0
0 stars 0 forks source link

Typos and improvement help and messages #85

Open buhtignew opened 4 months ago

buhtignew commented 4 months ago

I've decided to create this issue so in case there are typos or improvements to make in the help we can put everything here, so it doesn't interfere with the main testing process.

Right now I've found the following typo in transaction list help:

Only show received transactions (not in combination with -x, -n, -c, -b or -g). Only show received transactions (not in combination with -n, -c, -b or -g).

Seems like almost the same text is included into this line twice, probably the second sentence is the old one. _ If it can be useful I've found another one in the card list help. In the description sections is written:

    Usage:

    pacli card list
    pacli token list

While if I've got it right in the card list command has become token transfers command.

_ 1) In token transfers help there is the following line:

NOTES
    You can also use flags syntax for POSITIONAL ARGUMENTS

I don't know whether it's relevant in that context.

_ In the token list help there is the following explanation about the -o flag:

 Shows only the P2TH address of each token/deck. When used with -d, shows all P2TH addresses of the dPoD tokens.

However the both the token list -o and the token list -o -d commands output the same number of decks. I assume the -p flag was formerly -d in this case and the description wasn't changed after the change.

2) By the way the token list -o -b seems to work as well. Maybe we should mention that in the -o flag description as well?

3) I've also discovered that there is no P2TH address in the token list output. Thus it's not clear what is being this flag's output compared to (it's not something extracted from the token list output, for instance). I think I know what is the meaning though - the flag probably means p2th-addresses-only, but by putting "only" ahead of "p2th" the meaning seems to become less evident. For that reason I'd suggest us to consider the -l flag which would mean --list-p2th-addresses-of-the-decks. What do you think?

_ Like in the issue #87 there is no difference between token list, token list -d, token list -q and token list -d -q, which is probably expected.

_ As mentioned here I'd suggest to add an additional information into the transaction list's -l description. Something like: "The flag grants a quicker output in case the blocks were stored using deck/token cache/init or address cache commands previously, otherwise it will be ignored".

_ The pacli token balances [ADDRESS|-w] usage mode description states:

Shows balances of all tokens, either on the specified address or on the whole wallet (with -w flag).
    If ADDRESS is not given and -w is not selected, the current main address is used.

It seems to me to remember that previously it was mentioning that this usage mode outputs the main dPoD and PoB desks info only.

_ The -a flag output for the token balances [ADDRESS|-w] -t DECK usage mode is the same as without the -a flag. Would it make sense to edit the -a flag description from See above. Shows balances of all tokens in JSON format. Not in combination with -o. into See above. Shows balances of all tokens in JSON format. Not in combination with -o and -t.?

d5000 commented 4 months ago

First error (token transfers) was already changed in an earlier commit.

  1. Please ignore this for now, it can't be changed. 1b. You're correct, this was an error, fixed it to -p.
  2. -o works with all types of decks. The addition is only because dPoD decks have several P2TH addresses.
  3. Don't agree because -o only lists P2TH addresses, the deck ID is only shown to identify the deck.
  4. -d and -q are only used in some of the options, in the others they won't have effect, but as mentioned elsewhere -d always shows a traceback when there is an error.
  5. (transaction list mention of speedup for block locators): Done.
  6. (token balances) You're correct the current description fits the -a mode. I added the mode without -a.
  7. (change for -a flag) Makes sense, changed.

Fixes are in commit 709830a.

buhtignew commented 4 months ago

I've got all the above. Letting this issue open to add other small notes of the kind.

buhtignew commented 4 months ago

The token init help describes the -a flag as following: In combination with -s, store blockheights for all initialized tokens/decks. Probably the -s flag should be edited into -c flag in that description.

_

Relatively to the new -f flag introduced here for the address cache and token cache commands maybe we can add in the flag's description: "Has a priority against the -b flag - if used both the -b flag is ignored.".

I've just discovered that the address set -f command not only creates a fresh address but also set it as a main one, maybe we should consider to change the message that is being output during the fresh address creation from Address already is saved in your wallet, ready to use. into Address already saved in your wallet and set as main.? I'd suggest us to add in the -f flag description of the token show command that it's incompatible with other flags, in case the -f flag is used all the others (I can't say it for sure for the -q and -d flags though) are ignored.

d5000 commented 3 months ago

All done in commit 7166bef.

buhtignew commented 3 months ago

Regarding you commit https://github.com/slimcoin-project/pacli/commit/10dc4a0434bea95b2158a82f9e388aff246c7e09 mentioned here maybe we could add the description of token list -s -p and token list -s -b mode of use in the token list help.

d5000 commented 3 months ago

Done, will be uploaded in the next update.

buhtignew commented 3 months ago

Regarding your reply here, maybe we can include that info into the token transfers and card list help, specifically in the Usage mode section. If I've got it right the above commands are not usable without the token_id, label or name, while from the help as we have it now it seems like they are.

_ Maybe we can improve the help of token transfers in the Name section. Editing List all transactions (cards, i.e. issues, transfers, burns) of a token (with support for deck labels). into List all existing transactions (issues, transfers, burns) of a token. The issue I'm trying to solve here is to make it clear that it's not only in-wallet transactions but basically all transactions registered on the net.

d5000 commented 3 months ago

You're correct, the TOKEN positional argument was missing as mandatory, I added it for the next update.

Second addition also makes sense, added it.

buhtignew commented 3 months ago

Second addition also makes sense, added it.

I still don't understand the meaning of the word cards in the Name section of our token transfers help: pacli token transfers - List all existing transactions (cards, i.e. issues, transfers, burns) of a token.

d5000 commented 3 months ago

A card (short form for CardTransfer) is an unit used for any kind of token transaction (issuance, transfer or burn transaction). There can however be more than one card in one Slimcoin transaction. For example when you transfer tokens to two recipients, this will be counted as two cards/CardTransfers.

buhtignew commented 3 months ago

I've got the meaning now. Wouldn't it make sense to change card into CardTransfer in that sentence for those like me who wouldn't be enough conscious of the short form of the term? But does the issuance transaction make part of CardTransfer or it's a CardIssue transaction? I agreement to your replies here and here, wouldn't it make sense specifying in relative help that the True and False words are case sensitive, or it's somehow obvious given the context? We've edited "In advanced mode" into "In advanced mode (-a)" for the -w flag of the address list command. Maybe we should do the same for the -o flag as well. For the better clarity I'd suggest us to edit the line "Without flags, sets the LABEL as the main address." into "Without flags, sets the address that has that LABEL as the main address." While testing pobtoken burn_coins command it was not enough clear for me where the burned amount goes if no deck is mentioned (see here). Then I've realized the standard PoB deck is being used in that case. Maybe we can make it explicit for the pacli pobtoken burn_coins AMOUNT usage mode? While I've been running the transaction list command I've noticed that the message Locator mode not supported if no addresses or decks are selected. Locators will not be used. is always displayed in case the -x flag is used without an address or a deck, regardless whether the command is launched with the -l flag or not. Maybe we can display it only when there is an -l flag as well. In pobtoken burn_coins help there is the following line: Burns coins checking for compatibility (e.g. deadlines) with token (deck) TOKEN. I understand the meaning, but I was wondering whether we can find a better wording for the end of the sentence. Maybe it can be changed basically into this: Burns coins checking for compatibility (e.g. deadlines) with token/deck TOKEN.? _ The same for the pobtoken create_tx we can change Send coins to the gateway (e.g. donation, investment) address of token (deck) TOKEN. into Send coins to the gateway (e.g. donation, investment) address of token/deck TOKEN. if you like it. _ The usage modes of the pobtoken create_tx help is mentioning attoken instead of pobtoken, maybe we should edit it for better understanding? IDK whether it would make sense for the average user, but during my testing I've realized that it would be useful to have the address list displaying the block on which the data is displayed. In agreement to the issue #142 where the use of the global labels for the pobtoken burn_coins command with the -i flag (pobtoken burn_coins 1 -i ATTokenNewSpec, for instance) was implemented and since the local labels were already usable with that flag I'd suggest us to make some modification of the -i flag description and maybe of the extended form of the flag itself as well. I.e. we can change --idstr into --id_or_label and its description can be changed from ID of the Token or Deck you want to check compatibility with. into ID or label of the Token/Deck you want to check compatibility with. I haven't included this info into the issue #162, but the attoken claim command complains about ambiguity if run it with the -d flag. I was thinking whether it would make sense to change the name of the category at_address in the token show TOKEN -f -i output (or in the token info TOKEN if we decide to opt for the solution expressed in the issue #159) into gateway_address in order to have it aligned with the terminology (the flag -g) we've introduced for the transaction list command. After I've understood, thanks to your explanation here, on how the decks with the same global name are being managed by the code I've begun noticing the message More than one matching deck found: included in the outputs where the ambiguity occurs. I also remember we've decided in the past to treat the older decks as the decks that were really meant by the user, but as we've seen in the issue mentioned above sometimes it can happen that the older doesn't mean the right one, so I was wondering whether it would make sense to just stop executing command in case of ambiguity and output the message Error: deck's global name ambiguity, please use the meant deck's id or its local label. just after the More than one matching deck found: message. Another thing we should be careful about is that since the local label is being automatically copied from the global name during the deck initialization, in case of the same global name used twice or more for different decks there is at least one local label that is exactly the same as those multiple global names. So it's possible that the user means the local label, but the code means the global names which could lead the users to misunderstand the messages our code outputs, even if the user is trying to use the local labels only while working with the decks. Maybe we should consider to add some suffix to the local labels when they are copied from the global names (something like GLOBALNAME_l, for instance) However knowing that general rule the scammers may create decks that have that suffix in their names in order the user to send funds to the wrong deck while using the local label (since the global name have the priority). For that reason we probably should also make a check between the local labels and the global names and if there are matches the code should output the error message and just stop executing the command it was about to run. _ If we decide to add a suffix to the local labels created by the pobtoken deck_spawn command we may also implement it for the attoken deck_spawn. I think it would be comfortable if the address set command had an output, the address show -l output would be fine for instance, because with the quite output we have right now it's not clear whether the change of the main address has really happened or the command hasn't done anything. So each time I run address set I also run address show -l to check whether everything is fine. Maybe it would even make sense to add the address balance output as well. It seems like the local addresses' labels are not usable in case of the claim requests for the multiple receivers (for instance as mentioned in #165 it's not possible to run the attoken claim johns_attoken2 c6a73f51fc3d336689e43c60a6715110486fca80b9940cafb38c6ddba0b90ddd -r "[usage_checked, old_default, new_after_refill9, fresh_after_refill9, flag-l]" -a "[0.7, 0.1, 0.01, 0.3, 0.02]" command). I don't know whether it would make sense to additionally stress this fact in the help of the attoken claim, pobtoken claim and podtoken claim commands. _ I've run attoken create_tx 3f5920b3b26ca8089479691d2900d7d31970013b5cb874194876e4cebca02156 1.1234567890 and got the following message:

Error: End deadline for burn or gateway transactions of this token is is block 505942.

Maybe we can edit it into `Error: Final deadline for the burn or gateway transactions of this token is the block 505942.'?

buhtignew commented 1 month ago

The previous post has become too long. I'm opening a new one. Maybe we can consider editing the line Specify a multiplier for the reward.. into Specify an integer multiplier for the reward. in the help of the pobtoken spawn and attoken spawn command? Since the name of the deck_spawn was renamed (as mentioned here) into spawn for the pobtoken and attoken groups, we can also edit deck_spawn into spawn in the attoken spawn -h output (there is still the old term in the Description section). _ Would it make sense editing pacli dex exchange DECK PARTNER_ADDRESS PARTNER_INPUT CARD_AMOUNT COIN_AMOUNT line into pacli dex exchange DECK PARTNER_ADDRESS PARTNER_INPUT TOKEN_AMOUNT COIN_AMOUNT both in dex exchange -h Synopsis and Description? _ Would it make sense editing the word AMOUNT both in Synopsis as in Description of the dex lock -h into TOKEN_AMOUNT? _ I've run both proposal list 3369f3eb703656c0b46b613dcfb4df78fc05657a0b75f44551634de6321d9d9c -o and proposal list a2459e054ce0f600c90be458915af6bad36a6863a0ce0e33ab76086b514f765a -o and got:

No active and/or completed proposal states found for deck

However if I run the same commands without the -o flag I can see some completed proposal. Would it make sense making the message more specific in this case, i.e. No active proposal states found for deck? _ We probably should edit pacli proposal period, pacli proposal period -a, pacli proposal period PERIOD and pacli proposal period -b BLOCK into pacli proposal period FULL_PROPOSAL_ID PERIOD, pacli proposal period FULL_PROPOSAL_ID -a and pacli proposal period FULL_PROPOSAL_ID -b BLOCK in proposal period -h Usage options section. _ Maybe we could edit Show a parameter of the proposal state dictionary (only in combination with -s) into Only in combination with -s. Extract the parameter value(s) from the output provided by proposal show command if run with the -s flag only. (or something similar) in the proposal show -h. At least as you can see from the issue #179 which I've solved by myself it was not that clear to me the usage of that flag, because the -s flag follows the -p flag in the Flags section of the help so the expression proposal state dictionary was not that clear to me when I've begun testing the -s flag. Another idea can be at least to rename the --state flag into --state_of_proposal _ Maybe we can edit pacli proposal show LABEL_OR_ID -s [-p PARAM] [-a] into pacli proposal show LABEL_OR_ID -s [-p PARAM |-a|-b] in the Usage modes section of the proposal show -h output, since the -b flag is also to be used with the -s flag only and all the three flags (-p, -a and -b) can't be used together.

buhtignew commented 1 month ago

I'm beginning with another post, since the previous one was becoming too long to manage. _ In agreement to this maybe we can add in the proposal list -h the same info already added for the podtoken state -h and podtoken votes -h: If deck is not specified, the default dPoD deck is used.

d5000 commented 2 weeks ago

I'm currenlty working on the first long batch of suggestions (on a whole I agree with them and have modified the docstrings accordingly) and stumbled upon this, which is a misunderstanding:

While testing pobtoken burn_coins command it was not enough clear for me where the burned amount goes if no deck is mentioned (see https://github.com/slimcoin-project/pacli/issues/138). Then I've realized the standard PoB deck is being used in that case.

Burning coins is "neutral", a burn transaction is valid for all PoB token decks. It even gives you proof-of-burn "score" for the slimcoin algorithm. It doesn't matter if you use the pacli command or slimcoind burncoins.


Another comment: In reality I like the token (deck) wording more, because token/deck would suggest that token is a complete synonym for deck. It is true that token commands can replace deck commands but the other way around is not true. I'll try to be consistent with that wording though.


at_address is used because it is a system parameter of this dictionary. It stands both for gateway and burn address.


So it's possible that the user means the local label, but the code means the global names

Local labels have always priority so this will not occur. When the label is first stored when deck is initialized, the code checks for local labels if they exist, and if not it puts out a message inviting the user to select the deck's label himself.


Some other suggestions were already solved (e.g. the ambiguity of -d in attoken claim and the local labels support in receiver lists of the claim commands)