BM-3: feat(epoching): Epoching module setup #2

SebastianElvis · 2022-06-08T01:15:39Z

Fixes BM-11 under BM-3.

This PR adds the vanilla epoching module.
It makes the following modifications atop main:

Added .gitignore, adapted from https://github.com/osmosis-labs/osmosis/blob/main/.gitignore
Added config.yml, adapted from https://github.com/osmosis-labs/osmosis/blob/main/config.yml (without this ignite chain serve will prompt errors)
Added x/epoching/ and proto/babylon/epoching/ generated from ignite scaffold module epoching, with minor modfications for compilation
Added dependency of x/epoching/ to app/

Future work on the epoching module includes other child issues in https://babylon-chain.atlassian.net/browse/BM-3 and will be done in separate PRs.

aakoshh

That's an impressive amount of boilerplate out of the way! Nice work, it will surely make the rest of the PRs easier to grasp 👏

I wonder why the whole app.go file shows as each line has changed. Did you insert your edits manually, or was it somehow generated by a tool? How about the rest of the files?

I only have some clarification questions.

aakoshh · 2022-06-09T19:26:22Z

proto/babylon/epoching/genesis.proto

+// GenesisState defines the epoching module's genesis state.
+message GenesisState {
+ Params params = 1 [(gogoproto.nullable) = false];
+ // this line is used by starport scaffolding # genesis/proto/state


Just looking at https://github.com/cosmos/cosmos-sdk/blob/v0.45.4/proto/cosmos/staking/v1beta1/genesis.proto#L16-L38 it appears like they make room in the genesis file to carry over state from a previous chain, like a hard fork. For example unbonding_delegations would not make sense at the very first genesis, surely nobody is unbonding yet, but if we want to restart a new chain from the ashes of the old, it can be used to restore state.

So I wonder if there should be similar entries for delayed staking transactions and BLS keys here.

Agree. In the epoching module such historical states may include the delayed messages and the epoch number (if the epoch length is a variable).

x/epoching/keeper/params.go

aakoshh · 2022-06-09T19:41:34Z

x/epoching/types/keys.go

+
+const (
+ // ModuleName defines the module name
+ ModuleName = "epoching"


Hopefully this will never clash with the one in the cosmos SDK. Like during a migration, if we want to enable that one in the future.

In that case perhaps we can rename our epoching module as something like bblepoching when importing it.

Let's leave it for now, so it matches, then we'll see. In Osmosis these are called epochs.

aakoshh · 2022-06-09T19:45:06Z

x/epoching/types/keys.go

+
+)
+
+


Should we have a prefix like https://github.com/osmosis-labs/osmosis/blob/main/x/epochs/types/keys.go#L18?

app/app.go

aakoshh · 2022-06-09T20:02:06Z

app/app.go

+ app.mm.SetOrderEndBlockers(
+ crisistypes.ModuleName, govtypes.ModuleName, stakingtypes.ModuleName,
+ capabilitytypes.ModuleName, authtypes.ModuleName, banktypes.ModuleName, distrtypes.ModuleName,
+ slashingtypes.ModuleName, minttypes.ModuleName,
+ genutiltypes.ModuleName, evidencetypes.ModuleName, authz.ModuleName,
+ feegrant.ModuleName,
+ paramstypes.ModuleName, upgradetypes.ModuleName, vestingtypes.ModuleName,
+ /*
+ TODO: add bbl module
+ testbblmoduletypes.ModuleName,
+ */
+ epochingtypes.ModuleName,
+ )


Maybe worth leaving a comment that if we don't want staking to run at the end of each block, we should remove stakingtypes.ModuleName from app.mm.OrderEndBlockers after this call (it's a public field).

One possible issue with the way it is now is if in the last block where we want to run epoching.EndBlock, if that returns a non-empty set of validator updates, and the staking.EndBlock also returns a non-empty set, then Tendermint will panic. It only allows one module to return a changeset. With the current order, staking might return a changeset if there was slashing.

So we can either:

Never call staking.EndBlock, only epoching.EndBlock

Make sure we call epoching.EndBlock first but only to dequeue the delayed staking requests, and then let staking.EndBlock take care of executing them and return the changeset.

But because we only want the power transfer to happen, not the processing of the unbonding queues, we probably can't allow staking to run as-is.

SebastianElvis · 2022-06-09T23:49:10Z

I wonder why the whole app.go file shows as each line has changed. Did you insert your edits manually, or was it somehow generated by a tool? How about the rest of the files?

I inserted the lines in app.go and app_test.go manually according to the comments. It seems that throughout the current code base, app.go and app_test.go are the only two files that require modifications when adding a new module.

SebastianElvis · 2022-06-10T00:16:31Z

Thanks for the review Akosh! I have fixed or replied the comments. Feel free to re-review.

aakoshh

Cool, thanks for following up on all my comments 👍

vitsalis · 2022-06-10T08:53:27Z

x/epoching/keeper/msg_server.go

+)
+
+type msgServer struct {
+ Keeper


This shoud be a reference to a keeper k Keeper.

Good catch!

SebastianElvis · 2022-06-10T08:55:06Z

proto/babylon/epoching/genesis.proto

@@ -0,0 +1,14 @@
+syntax = "proto3";
+package babylonchain.babylon.epoching;


Suggested change

package babylonchain.babylon.epoching;

package babylon.epoching;

Or babylon.epoching.v1 if we want versioning for those.

vitsalis · 2022-06-10T16:27:09Z

Maybe we should also remove the comments related to ignite scaffolding.

SebastianElvis · 2022-06-13T00:54:24Z

Maybe we should also remove the comments related to ignite scaffolding.

Can we keep them at the moment? Just in case if we want to create stuff (CLI, message types, interfaces) by using ignite.

SebastianElvis · 2022-06-13T02:21:23Z

Thanks for the comments Vitalis! I have addressed them and rebased this PR wrt main. Feel free to re-review.

vitsalis

LGTM!

SebastianElvis added 4 commits June 7, 2022 16:06

init

ee50f30

vanilla epoching module

8eaf807

add epoching dependency to App

9a46cf3

housekeeping stuff

f2a93cb

SebastianElvis requested review from aakoshh and gitferry and removed request for aakoshh and gitferry June 8, 2022 01:55

SebastianElvis added 2 commits June 8, 2022 12:12

fix protobuf file directory

3b10e66

formatting stuff

491a3c5

KonradStaniec mentioned this pull request Jun 9, 2022

BM-6: Add initial scaffold for rawcheckpoint module #4

Merged

aakoshh changed the title ~~feat(epoching): Epoching module setup~~ BM-3: feat(epoching): Epoching module setup Jun 9, 2022

aakoshh reviewed Jun 9, 2022

View reviewed changes

SebastianElvis added 2 commits June 10, 2022 10:01

fix comments

424a049

comments

f416b7c

SebastianElvis requested a review from aakoshh June 10, 2022 06:18

aakoshh approved these changes Jun 10, 2022

View reviewed changes

vitsalis reviewed Jun 10, 2022

View reviewed changes

SebastianElvis commented Jun 10, 2022

View reviewed changes

SebastianElvis added 4 commits June 13, 2022 10:55

Merge branch 'main' into add-epoching-module

a1cc25d

fix msgServer

1cf7b1e

minor

db5565a

v1 for proto

73809ba

SebastianElvis requested a review from vitsalis June 13, 2022 02:21

minor

c2dac96

vitsalis approved these changes Jun 13, 2022

View reviewed changes

SebastianElvis merged commit f91eedc into main Jun 13, 2022

SebastianElvis deleted the add-epoching-module branch June 13, 2022 11:19

KonradStaniec pushed a commit that referenced this pull request Oct 25, 2023

btcstaking: DB schema (#2)

dc94fab

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BM-3: feat(epoching): Epoching module setup #2

BM-3: feat(epoching): Epoching module setup #2

SebastianElvis commented Jun 8, 2022 •

edited

Loading

aakoshh left a comment

aakoshh Jun 9, 2022

SebastianElvis Jun 9, 2022

aakoshh Jun 9, 2022

SebastianElvis Jun 10, 2022

aakoshh Jun 10, 2022

aakoshh Jun 9, 2022

aakoshh Jun 9, 2022

SebastianElvis commented Jun 9, 2022

SebastianElvis commented Jun 10, 2022

aakoshh left a comment

vitsalis Jun 10, 2022

aakoshh Jun 10, 2022

SebastianElvis Jun 10, 2022

vitsalis Jun 11, 2022

vitsalis commented Jun 10, 2022

SebastianElvis commented Jun 13, 2022

SebastianElvis commented Jun 13, 2022 •

edited

Loading

vitsalis left a comment

		@@ -0,0 +1,14 @@
		syntax = "proto3";
		package babylonchain.babylon.epoching;

	package babylonchain.babylon.epoching;
	package babylon.epoching;

BM-3: feat(epoching): Epoching module setup #2

BM-3: feat(epoching): Epoching module setup #2

Conversation

SebastianElvis commented Jun 8, 2022 • edited Loading

aakoshh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianElvis commented Jun 9, 2022

SebastianElvis commented Jun 10, 2022

aakoshh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitsalis commented Jun 10, 2022

SebastianElvis commented Jun 13, 2022

SebastianElvis commented Jun 13, 2022 • edited Loading

vitsalis left a comment

Choose a reason for hiding this comment

SebastianElvis commented Jun 8, 2022 •

edited

Loading

SebastianElvis commented Jun 13, 2022 •

edited

Loading