Possible typo and clarification edits on README.md

[dkgaraujo]

Hi all. I would like to propose the following edits to README.md. They are minor in nature, but should be helpful to users first getting in contact with this repo.

• In the Run the code section, I believe there should be a hyphen between "docker" and "compose", like so:

Run the System

# docker-compose --file docker-compose-2pc.yml up --build

• In the Setup test wallets and test them section, I would propose adding a one-row explainer that the references to wallet1 are the result of the previous step. Users replicating this code locally might not be aware of that and think that their code is wrong for some reason, when in fact it's just a matter of substituting the string in the instructions for whatever string was the output in their own local version.

[HalosGhost]

In the Run the code section, I believe there should be a hyphen between "docker" and "compose", like so:

# docker-compose --file docker-compose-2pc.yml up --build

Actually, the use of docker compose is intentional. compose v2 is integrated into docker as a subcommand (as opposed to docker-compose which was a standalone binary). cf. the docs. In our case, docker-compose actually has different behavior which is more likely to lead to breakage and confusion, so we specifically reference v2.

In the Setup test wallets and test them section, I would propose adding a one-row explainer that the references to wallet1 are the result of the previous step. Users replicating this code locally might not be aware of that and think that their code is wrong for some reason, when in fact it's just a matter of substituting the string in the instructions for whatever string was the output in their own local version.

I'm not actually quite sure what you're referring to. The first command shown under the section you mention actually creates the new wallet and mempool (which the user can name arbitrarily on the command-line; we gave them representative names in the example to try to make it more clear), the wallets for testing are not created as a result of the previous section. You can actually see a demonstration of this in the example output of that first command (copied here for ease):

# ./build/src/uhs/client/client-cli 2pc-compose.cfg mempool0.dat wallet0.dat mint 10 5
[2021-08-17 15:11:57.686] [WARN ] Existing wallet file not found
[2021-08-17 15:11:57.686] [WARN ] Existing mempool not found
4bc23da407c3a8110145c5b6c38199c8ec3b0e35ea66bbfd78f0ed65304ce6fa

The client warns that mempool0.dat and wallet0.dat don't exist (so it has to create them).

[dkgaraujo]

Thanks for the quick feedback @HalosGhost!

On the docker composer topic: thanks for clarifying, makes complete sense!

About the wallet issue - I'm afraid I didn't express myself as clearly as I wanted to. What I mean is that between the steps where we make a wallet and then we send 30 units to that new wallet, the command includes as argument the payee's wallet string (usd1qrw038lx5n4wxx3yvuwdndpr7gnm347d6pn37uywgudzq90w7fsuk52kd5u in the example). But every time this is run, this string changes, so uses that want to follow along this walkthrough would also need to change the string they are referring to in the step "Send currency from one wallet to another". This is an obvious thing but is not explicitly mentioned, so I thought including a quick sentence such as the following in bold would address the issue:

Send currency from one wallet to another (e.g., 30 atomic units of currency). Note that you may need to change the wallet string ("usd1qrw0...") depending on the result of the previous step

Not a hugely important issue, just a suggestion to make the introduction more clear to newly interested people looking to familiarise themselves with this project.

[HalosGhost]

Ahh, I see what you're saying. Namely, you're saying that in the send-currency step, it doesn't feel clear that the string you pass is the address generated in the previous step?

[dkgaraujo]

Yes, exactly.

[HalosGhost]

I'd happily review/accept any PR to improve our documentation (including README clarity)!

[maurermi]

@HalosGhost looks like #146 closes this issue

[HalosGhost]

@maurermi I think you're correct. Thank you for helping out on the clean-up!