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

Throttle ADR #682

Merged
merged 23 commits into from
Feb 9, 2023
Merged

Throttle ADR #682

merged 23 commits into from
Feb 9, 2023

Conversation

shaspitz
Copy link
Contributor

@shaspitz shaspitz commented Jan 27, 2023

Description

Removes throttle.md in favor of a more complete ADR for throttling

Linked issues

Closes: #615
See cosmos/ibc#924 for changes to spec needed for throttling

Type of change

  • Docs: Adds documentation

@shaspitz
Copy link
Contributor Author

shaspitz commented Feb 6, 2023

@ivan-gavran I've incorporated my understanding of your invariant derivation into this PR, hopefully the intuition section is now more understandable

@shaspitz shaspitz marked this pull request as ready for review February 6, 2023 21:50
@shaspitz shaspitz mentioned this pull request Feb 6, 2023
1 task
@ivan-gavran
Copy link
Contributor

@ivan-gavran I've incorporated my understanding of your invariant derivation into this PR, hopefully the intuition section is now more understandable

Cool! It is indeed much clearer now. I noticed a few things that could still be improved and created a PR since I don't have access to push directly. Ping me if you want to discuss the changes.

One question occurred to me: why not using Latex rendering that Github supports now for ADRs?

Finally, a bit of nitpicking: why calling the property you are describing an invariant? An invariant would be something that holds always (e.g., a simple one, it is always the case that the meter's value is smaller than its allowance), whereas what you are describing here is a property.

docs/architecture/adr-002-throttle.md Outdated Show resolved Hide resolved
docs/architecture/adr-002-throttle.md Outdated Show resolved Hide resolved
@shaspitz
Copy link
Contributor Author

shaspitz commented Feb 7, 2023

@ivan-gavran I've incorporated my understanding of your invariant derivation into this PR, hopefully the intuition section is now more understandable

Cool! It is indeed much clearer now. I noticed a few things that could still be improved and created a PR since I don't have access to push directly. Ping me if you want to discuss the changes.

One question occurred to me: why not using Latex rendering that Github supports now for ADRs?

Finally, a bit of nitpicking: why calling the property you are describing an invariant? An invariant would be something that holds always (e.g., a simple one, it is always the case that the meter's value is smaller than its allowance), whereas what you are describing here is a property.

Thanks for the feedback! On latex rendering, I wasn't aware of this, I'll take a look 👍

On property vs invariant, I agree with your suggestion and have changed instances of "invariant" to "property"

Copy link
Contributor

@jtremback jtremback left a comment

Choose a reason for hiding this comment

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

LGTM

@shaspitz shaspitz merged commit 5563395 into main Feb 9, 2023
@shaspitz shaspitz deleted the throttle-adr branch February 9, 2023 21:56
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.

No ADR for jail throttling
4 participants