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 Random Activity to README and CLI Options #125

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

carlaKC
Copy link
Contributor

@carlaKC carlaKC commented Oct 4, 2023

This PR is a small follow up for #113 adding:

  • The ability to configure multiplier + expected_payment_amount.
  • Moves "get started" up so that people don't get bogged down by the long config section when they first land.
  • Updated readme with sample configs.

@carlaKC carlaKC force-pushed the 72-configanddocs branch 3 times, most recently from 95362d9 to 06b45fd Compare October 4, 2023 18:54
@carlaKC
Copy link
Contributor Author

carlaKC commented Oct 4, 2023

Including the alias improvements in #103 in the readme here so that we can do all our fancy updates in one do, so this PR is a dependent.

@carlaKC carlaKC requested review from Extheoisah and sr-gi October 5, 2023 17:13
Copy link
Collaborator

@okjodom okjodom left a comment

Choose a reason for hiding this comment

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

:shipit: 👍🏿

Copy link
Member

@sr-gi sr-gi left a comment

Choose a reason for hiding this comment

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

Looks pretty good. I left some comments inline.

README.md Outdated
}
```

Nodes can be identified by an arbitrary string ("Alice", "CLN1" etc) or
Copy link
Member

Choose a reason for hiding this comment

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

missing comma after "CLN1"

README.md Outdated
will be sent by nodes, randomness will be introduced such that larger
nodes send a wider variety of payment sizes around this expectation.
* `--capacity-multiplier`: the number of times over that each node in
the network sends their capacity in a calendar month - 2 indicating
Copy link
Member

Choose a reason for hiding this comment

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

Using - style here reads pretty weird

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Meaning, don't use ` to make it a code block?

Copy link
Member

@sr-gi sr-gi Oct 5, 2023

Choose a reason for hiding this comment

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

Oh no, I mean "- 2 indicating ..."

Looks like you're talking to minus two

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah gotcha!

README.md Outdated
sim-cli --config config.json
```
Activity source nodes may be reference by id string
or public key (as specified in `nodes`), but destin
Copy link
Member

Choose a reason for hiding this comment

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

Is this an unfinished, probably old, rephased sentence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoop, yeah should be deleted!

@carlaKC
Copy link
Contributor Author

carlaKC commented Oct 5, 2023

Updated to include #130 as well

@carlaKC carlaKC force-pushed the 72-configanddocs branch 2 times, most recently from 9e719cd to 7066853 Compare October 5, 2023 20:23
Surface so that end users can configure these values. Note that
expected payment amount is intentionally called "avg" in the user
config because that's a more user friendly / recognizable term, but
we keep expected payment amount elsewhere in the code base since it's
the expectation we provide for our distribution.
Add sections for each "type" of config. Also move getting started up
so that we have something actionable before the long/difficult
configuration part.
Copy link
Member

@sr-gi sr-gi left a comment

Choose a reason for hiding this comment

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

LGTM

@carlaKC carlaKC merged commit 8714795 into bitcoin-dev-project:main Oct 5, 2023
2 checks passed
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.

3 participants