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

Moving voting power table from btcstaking module to finality module #24

Open
gitferry opened this issue Aug 19, 2024 · 1 comment · May be fixed by #217
Open

Moving voting power table from btcstaking module to finality module #24

gitferry opened this issue Aug 19, 2024 · 1 comment · May be fixed by #217
Assignees
Labels
enhancement New feature or request phase-2-readiness

Comments

@gitferry
Copy link
Member

context

@gitferry gitferry added phase-2-readiness enhancement New feature or request labels Aug 19, 2024
gitferry added a commit that referenced this issue Aug 22, 2024
…ed pub rand (#23)

This closes the second part of #8. In particular, this PR:
* Implemented `HasTimestampedPubRand` in the finality keeper to check
whether a pub rand at a given height is BTC-timestamped
* Added expected finality keeper to btcstaking keeper so that
`HasTimestampedPubRand` can be called within the module
* When signing voting power to fps in `btcstaking`'s begin blocker, if
the fp does not have timestamped pub rand, voting power will not be
assigned

Note that this PR introduced circular dependency between the btc staking
module and finality module, which should be addressed by moving the
voting power table to the finality module, tracked by issue #24
gitferry added a commit that referenced this issue Aug 27, 2024
…ed pub rand (#23)

This closes the second part of #8. In particular, this PR:
* Implemented `HasTimestampedPubRand` in the finality keeper to check
whether a pub rand at a given height is BTC-timestamped
* Added expected finality keeper to btcstaking keeper so that
`HasTimestampedPubRand` can be called within the module
* When signing voting power to fps in `btcstaking`'s begin blocker, if
the fp does not have timestamped pub rand, voting power will not be
assigned

Note that this PR introduced circular dependency between the btc staking
module and finality module, which should be addressed by moving the
voting power table to the finality module, tracked by issue #24
gitferry added a commit that referenced this issue Sep 2, 2024
…ed pub rand (#23)

This closes the second part of #8. In particular, this PR:
* Implemented `HasTimestampedPubRand` in the finality keeper to check
whether a pub rand at a given height is BTC-timestamped
* Added expected finality keeper to btcstaking keeper so that
`HasTimestampedPubRand` can be called within the module
* When signing voting power to fps in `btcstaking`'s begin blocker, if
the fp does not have timestamped pub rand, voting power will not be
assigned

Note that this PR introduced circular dependency between the btc staking
module and finality module, which should be addressed by moving the
voting power table to the finality module, tracked by issue #24
gitferry added a commit that referenced this issue Sep 2, 2024
…ed pub rand (#23)

This closes the second part of #8. In particular, this PR:
* Implemented `HasTimestampedPubRand` in the finality keeper to check
whether a pub rand at a given height is BTC-timestamped
* Added expected finality keeper to btcstaking keeper so that
`HasTimestampedPubRand` can be called within the module
* When signing voting power to fps in `btcstaking`'s begin blocker, if
the fp does not have timestamped pub rand, voting power will not be
assigned

Note that this PR introduced circular dependency between the btc staking
module and finality module, which should be addressed by moving the
voting power table to the finality module, tracked by issue #24
gitferry added a commit that referenced this issue Sep 3, 2024
…ed pub rand (#23)

This closes the second part of #8. In particular, this PR:
* Implemented `HasTimestampedPubRand` in the finality keeper to check
whether a pub rand at a given height is BTC-timestamped
* Added expected finality keeper to btcstaking keeper so that
`HasTimestampedPubRand` can be called within the module
* When signing voting power to fps in `btcstaking`'s begin blocker, if
the fp does not have timestamped pub rand, voting power will not be
assigned

Note that this PR introduced circular dependency between the btc staking
module and finality module, which should be addressed by moving the
voting power table to the finality module, tracked by issue #24
@SebastianElvis SebastianElvis self-assigned this Oct 15, 2024
@SebastianElvis
Copy link
Member

SebastianElvis commented Oct 22, 2024

Tasks breakdown

SebastianElvis added a commit that referenced this issue Oct 22, 2024
First step of #24

This PR moves the voting power distribution update algorithm to
`x/finality`. This is a major refactoring that includes

- moving `max_active_finality_providers` parameter to `x/finality`
- moving many functions in `x/btcstaking/power_dist_change.go` and
relevant tests to `x/finality`
- moving test utilities under `x/btcstaking/keeper_test.go` to a new
module `testutil/btcstaking-helper`, and use it for existing tests of
`x/btcstaking` and `x/finality`
- removing the cyclic dependency between `x/btcstaking` and
`x/finality`, as well as their hooks. Now the hooks in these 2 modules
are not necessary, as the only dependency between them is that
`x/finality` will invoke `x/btcstaking`, but not the other way

TODOs in subsequent PRs:

- moving some relevant query APIs to `x/finality`
- moving the voting power dist cache and voting power table KV stores to
`x/finality`
SebastianElvis added a commit that referenced this issue Oct 23, 2024
First step of #24

This PR moves the voting power distribution update algorithm to
`x/finality`. This is a major refactoring that includes

- moving `max_active_finality_providers` parameter to `x/finality`
- moving many functions in `x/btcstaking/power_dist_change.go` and
relevant tests to `x/finality`
- moving test utilities under `x/btcstaking/keeper_test.go` to a new
module `testutil/btcstaking-helper`, and use it for existing tests of
`x/btcstaking` and `x/finality`
- removing the cyclic dependency between `x/btcstaking` and
`x/finality`, as well as their hooks. Now the hooks in these 2 modules
are not necessary, as the only dependency between them is that
`x/finality` will invoke `x/btcstaking`, but not the other way

TODOs in subsequent PRs:

- moving some relevant query APIs to `x/finality`
- moving the voting power dist cache and voting power table KV stores to
`x/finality`
SebastianElvis added a commit that referenced this issue Oct 23, 2024
First step of #24

This PR moves the voting power distribution update algorithm to
`x/finality`. This is a major refactoring that includes

- moving `max_active_finality_providers` parameter to `x/finality`
- moving many functions in `x/btcstaking/power_dist_change.go` and
relevant tests to `x/finality`
- moving test utilities under `x/btcstaking/keeper_test.go` to a new
module `testutil/btcstaking-helper`, and use it for existing tests of
`x/btcstaking` and `x/finality`
- removing the cyclic dependency between `x/btcstaking` and
`x/finality`, as well as their hooks. Now the hooks in these 2 modules
are not necessary, as the only dependency between them is that
`x/finality` will invoke `x/btcstaking`, but not the other way

TODOs in subsequent PRs:

- moving some relevant query APIs to `x/finality`
- moving the voting power dist cache and voting power table KV stores to
`x/finality`
SebastianElvis added a commit that referenced this issue Oct 25, 2024
Second half of #24 

This PR moves the voting power table/cache and relevant APIs to
`x/finality`. This includes the following

- [x] move voting power table KV store to `x/finality`
- [x] move voting power dist cache to `x/finality`
- [x] move 4 APIs `ActiveFinalityProvidersAtHeight`,
`FinalityProviderPowerAtHeight`, `FinalityProviderCurrentPower`,
`ActivatedHeight` to `x/finality`
- [x] fix fuzz tests
- [x] fix e2e tests
- [x] fix doc
- [x] fix changelog

Note that this affects many LoCs since it touches a bunch of proto
files. The actual modification is much smaller than it looks like.
SebastianElvis added a commit that referenced this issue Oct 25, 2024
First step of #24

This PR moves the voting power distribution update algorithm to
`x/finality`. This is a major refactoring that includes

- moving `max_active_finality_providers` parameter to `x/finality`
- moving many functions in `x/btcstaking/power_dist_change.go` and
relevant tests to `x/finality`
- moving test utilities under `x/btcstaking/keeper_test.go` to a new
module `testutil/btcstaking-helper`, and use it for existing tests of
`x/btcstaking` and `x/finality`
- removing the cyclic dependency between `x/btcstaking` and
`x/finality`, as well as their hooks. Now the hooks in these 2 modules
are not necessary, as the only dependency between them is that
`x/finality` will invoke `x/btcstaking`, but not the other way

TODOs in subsequent PRs:

- moving some relevant query APIs to `x/finality`
- moving the voting power dist cache and voting power table KV stores to
`x/finality`
SebastianElvis added a commit that referenced this issue Oct 25, 2024
Second half of #24 

This PR moves the voting power table/cache and relevant APIs to
`x/finality`. This includes the following

- [x] move voting power table KV store to `x/finality`
- [x] move voting power dist cache to `x/finality`
- [x] move 4 APIs `ActiveFinalityProvidersAtHeight`,
`FinalityProviderPowerAtHeight`, `FinalityProviderCurrentPower`,
`ActivatedHeight` to `x/finality`
- [x] fix fuzz tests
- [x] fix e2e tests
- [x] fix doc
- [x] fix changelog

Note that this affects many LoCs since it touches a bunch of proto
files. The actual modification is much smaller than it looks like.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request phase-2-readiness
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants