chore: Improve external events format #169
Conversation
gitferry
changed the title
gitferry
requested review from
Lazar955,
RafilxTenfen,
KonradStaniec and
jrwbabylonlab
and removed request for
a team
gitferry
added
api breaking
gitferry
force-pushed
the
fix/external-events-format
branch
3 times, most recently
from
aa31ddb to
492a9be
Compare
|
It seems that the label should be |
|
@jrwbabylonlab does primitive numbers as strings also improve situation on api side ? From my point of view this is a bit bad 😅 as now I am looking at number in some events and see string and I do not not know:
I.e proto file loses any documentation qualities about numbers. EDIT: It seems cosmos-sdk use events this way, so probably every tooling here is expects string in events. Example: https://github.com/cosmos/cosmos-sdk/blob/main/x/staking/keeper/msg_server.go#L559. Lets leave it as it is 👍 |
gitferry
force-pushed
the
fix/external-events-format
branch
from
492a9be to
4fb9bfd
Compare
|
Just adding a note here as already discussed in DM: |
| // btc_pk_hex is the hex string of Bitcoin secp256k1 PK of this finality provider | ||
| string btc_pk_hex = 1 [(amino.dont_omitempty) = true]; | ||
| // commission defines the commission rate of the finality provider in decimals. | ||
| string commission = 2 [(amino.dont_omitempty) = true]; |
There was a problem hiding this comment.
why does commission is none-optional in the edit mode?
There was a problem hiding this comment.
commission is a required field when a fp is registered on babylon
There was a problem hiding this comment.
sure, but for the edit event, why does it need to be here? if FP's commission was not updated, we don't emit it, right?
| BTCDelegationStatus state = 4; | ||
| string end_height = 3 [(amino.dont_omitempty) = true]; | ||
| // new_state of the BTC delegation | ||
| string new_state = 4 [(amino.dont_omitempty) = true]; | ||
| } | ||
|
|
||
| // EventBTCDelgationUnbondedEarly is the event emitted when a BTC delegation | ||
| // is unbonded by staker sending unbonding tx to BTC | ||
| message EventBTCDelgationUnbondedEarly { |
There was a problem hiding this comment.
@gitferry just to confirm my assumption: we’re splitting the unbonded state into two separate events for natural unbonding and the unbonding path. This means the API no longer needs to differentiate between a transaction event and a block event.
There was a problem hiding this comment.
Yep, they are separate events EventBTCDelgationUnbondedEarly and EventBTCDelegationExpired but with the same state UNBONDED. They are both emitted as block events
| @@ -263,11 +263,11 @@ func (k Keeper) ProcessAllPowerDistUpdateEvents( | |||
| if _, ok := slashedFPs[fpBTCPKHex]; ok { | |||
| fp.IsSlashed = true | |||
|
|
|||
| statusChangeEvent := types.NewFinalityProviderStatusChangeEvent(fp.BtcPk, types.FinalityProviderStatus_STATUS_SLASHED) | |||
| statusChangeEvent := types.NewFinalityProviderStatusChangeEvent(fp.BtcPk, types.FinalityProviderStatus_FINALITY_PROVIDER_STATUS_SLASHED) | |||
| if err := sdkCtx.EventManager().EmitTypedEvent(statusChangeEvent); err != nil { | |||
There was a problem hiding this comment.
nit:
IMO, it might be more performant if each of the event type come with .toEvent method which will turn your custom event struct into the cosmos event type. https://pkg.go.dev/github.com/cometbft/[email protected]/abci/types#Event
Then use the EmitEvent instead of EmitTypedEvent to save some json marshal/unmarshl
There was a problem hiding this comment.
Not sure this is good to use EmitEvent because cosmos has listed EmitEvent as deprecated https://github.com/cosmos/cosmos-sdk/blob/v0.50.6/types/events.go#L45-L49
This PR flattened the external events and changed all the event fields to string so that it's easier for subscribers to parse them
| // ACTIVE -> INACTIVE. | ||
| // Note that it is impossible for a SLASHED finality provider to | ||
| // transition to other status | ||
| message EventFinalityProviderStatusChange { |
There was a problem hiding this comment.
@gitferry ouch. we still using status here. but anyway, not a big deal. let's leave it
This PR flattened the external events and changed all the event fields to string so that it's easier for subscribers to parse them