feat(x/gov): dynamic deposit increases only with proposal activation, and decreases only with time #105
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Do you want to update the ADR in this PR or should we do it in a separate one ? I would rather see the change in this PR so everything is bundled. |
Yes I was thinking the same |
tbruyelle
reviewed
Mar 27, 2025
docs/architecture/adr-003-governance-proposal-deposit-auto-throttler.md
Outdated
Show resolved
Hide resolved
docs/architecture/adr-003-governance-proposal-deposit-auto-throttler.md
Outdated
Show resolved
Hide resolved
| In a naive implementation for time-based updates every $T$ time has elapsed (a *tick*) the current value of $n_t$ should be used to calculate $D_t$. Having the accurate $D_t$ available to query at all times is important to be able to correctly submit a proposal. | ||
| But in reality, all we need to know is when $n_t$ **actually** changed the last time (which happens when proposals enter or leave the active proposals queue, and in itself also trigger a `MinDeposit` update), and how much time has passed since then, anytime the $D_t$ value is requested. | ||
| In a naive implementation for time-based decreases every $T$ time has elapsed (a *tick*) the current value of $n_t$ should be used to calculate $D_t$. Having the accurate $D_t$ available to query at all times is important to be able to correctly submit a proposal. | ||
| But in reality, all we need to know is when $n_t$ **actually** changed the last time (which happens when proposals enters the active proposals queue, and in itself also trigger a `MinDeposit` increase), and how much time has passed since then, anytime the $D_t$ value is requested. |
There was a problem hiding this comment.
I think this optimization has been discarded right ?
There was a problem hiding this comment.
No need to remove it from the ADR though. The ADR just states it's possible, and present the mathematical relation
There was a problem hiding this comment.
In fact, I removed the section talking about it in the implementation.
…ottler.md Co-authored-by: Thomas Bruyelle <thomas.bruyelle@tendermint.com>
…ottler.md Co-authored-by: Thomas Bruyelle <thomas.bruyelle@tendermint.com>
State was not reinitialized between test cases, which was confusing for some scenario and forced sometimes to reset things like the deposit throttling. The fix consists only of calling `suite.reset()` in `KeeperTestSuite.SetupSubTest()`, the other changes removes any correlation previously made by the test cases about this common state.
tbruyelle
approved these changes
Mar 28, 2025
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is based on #104
Based on discussions, this is a revised model for the dynamic deposit that:
Plus, remove the sensitivity to the distance for increases.
This PR should be merged only after #104 is (or in its stead). Moreover, if this PR is accepted, the partially overlapping PR #103 can be closed.
Importantly, this PR does not yet update neither the
x/govREADME.md nor the Dynamic Deposit ADR to reflect these changes, so it needs to be done.