feat: use typed events #1030

rahulghangas · 2022-11-17T07:12:57Z

switch to typed event for blob module'
~~Refactor integration test~~
Closes Switch usage from EmitEvent to EmitTypedEvent #968

codecov-commenter · 2022-11-17T07:25:09Z

Codecov Report

Merging #1030 (3f5bd02) into main (2f89956) will decrease coverage by 0.03%.
The diff coverage is 11.11%.

@@            Coverage Diff             @@
##             main    #1030      +/-   ##
==========================================
- Coverage   50.50%   50.46%   -0.04%     
==========================================
  Files          71       71              
  Lines        4390     4391       +1     
==========================================
- Hits         2217     2216       -1     
- Misses       1946     1947       +1     
- Partials      227      228       +1

Impacted Files	Coverage Δ
x/blob/types/events.go	`0.00% <0.00%> (ø)`
x/blob/keeper/keeper.go	`80.00% <25.00%> (-11.31%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

x/blob/keeper/keeper.go

vidhanarya · 2022-11-18T05:23:34Z

proto/blob/event.proto

+
+// EventPayForBlob defines an event that is emitted with the signer and blob size
+// at the end of a pay for blob txn
+message EventPayForBlob {


So we are changing event.type from payforblob to blob.EventPayForBlob or am I missing something here? If we are changing event type then relevant test (TestSubmitWirePayForBlob) in integration_test.go can be updated.

Yes, there's a pending task in this PR for the same

My bad, I overlooked the draft status and checklist (used to GitLab PRs 😅 ). My apologies 🙏

rahulghangas · 2022-11-18T05:41:59Z

@evan-forbes since we're changing the event name, in order to follow convention from the typed events ADR in cosmos-sdk, this will be a breaking change

rach-id

Good catch 👍 the older way was deprecated.

proto/blob/events.proto

x/blob/keeper/keeper.go

rach-id · 2022-11-18T12:20:11Z

x/blob/keeper/keeper.go

@@ -61,9 +61,12 @@ func (k Keeper) PayForBlob(goCtx context.Context, msg *types.MsgPayForBlob) (*ty
 	gasToConsume := uint64(shares.MsgSharesUsed(int(msg.BlobSize)) * GasPerMsgShare)
 	ctx.GasMeter().ConsumeGas(gasToConsume, payForBlobGasDescriptor)

-	ctx.EventManager().EmitEvent(
+	err := ctx.EventManager().EmitTypedEvent(
 		types.NewPayForBlobEvent(sdk.AccAddress(msg.Signer).String(), msg.GetBlobSize()),


Suggested change

types.NewPayForBlobEvent(sdk.AccAddress(msg.Signer).String(), msg.GetBlobSize()),

types.NewPayForBlobEvent(msg.Signer, msg.GetBlobSize()),

I guess the msg.Signer already contains the account address in Bech32 format. What do you think?

second this, we should be able to just use the signer. We can test manually by looking at the events in one of the integration tests

It doesn't look like that the conversion is as is, I'll confirm via a test

@rahulghangas Did you manage to check if it is the same?

@sweexordious msg.Signer is the raw form while the .String() method encodes it in bech32. Removing the above logic will be breaking change

I tried an example and apparently the msg.Signer contains:

cosmos1n5c5wmz5s2n0d8zpr5ldcl45nkjclaqktls7gd

while sdk.AccAddress(msg.Signer).String() returns:

cosmos1vdhhxmt0wvckudtrx4mk6734wvexuvry8pa8qu34d3jxxmp5x4hxk6nrd3shz6m5d3enwemycqe9zf

Which one do we want?

After we align on one, should we add a unit test that verifies an event is emitted with the appropriate signer?

I'm leaning towards first, since it's more compact and helps us reduce on-chain storage

rootulp

Refactor integration test

Where does this happen? It doesn't appear applicable to this PR.

evan-forbes · 2022-11-21T22:08:42Z

what's left for this? just reviewer feedback?

rahulghangas · 2022-11-22T06:04:20Z

Where does this happen? It doesn't appear applicable to this PR.

It doesn't, I've removed it from the checklist. Initially the idea was to change the test because the attribute key was hardcoded, but I've made it such that it infers the type name from the struct itself

rootulp

[not blocking] can we please update this section

celestia-app/x/blob/README.md

Lines 30 to 32 in 29d47af

    
           ## Events 
        
           - [`NewPayForBlobEvent`](https://github.com/celestiaorg/celestia-app/pull/213/files#diff-1ce55bda42cf160deca2e5ea1f4382b65f3b689c7e00c88085d7ce219e77303dR17-R21) is emitted with the signer's address and size of the message that is paid for.

to remove the permalink to the outdated event? Proposal:

## Events

- `PayForBlobEvent` is emitted with the signer's address and size of the message that is paid for.

Alternatively, we can also copy something like this: https://github.com/cosmos/cosmos-sdk/blob/16c772d900f530d16518491b19f60cfef9d713af/x/bank/README.md?plain=1#L339-L353

proto/blob/events.proto

… hardcoded

x/blob/README.md

x/blob/types/events.go

rootulp · 2022-11-29T18:21:58Z

x/blob/keeper/keeper.go

@@ -61,9 +61,12 @@ func (k Keeper) PayForBlob(goCtx context.Context, msg *types.MsgPayForBlob) (*ty
 	gasToConsume := uint64(shares.MsgSharesUsed(int(msg.BlobSize)) * GasPerMsgShare)
 	ctx.GasMeter().ConsumeGas(gasToConsume, payForBlobGasDescriptor)

-	ctx.EventManager().EmitEvent(
+	err := ctx.EventManager().EmitTypedEvent(
 		types.NewPayForBlobEvent(sdk.AccAddress(msg.Signer).String(), msg.GetBlobSize()),


After we align on one, should we add a unit test that verifies an event is emitted with the appropriate signer?

evan-forbes

LGTM!! thanks for this @rahulghangas

reviewer is OOO

- [x] switch to typed event for `blob` module' - [ ] ~~Refactor integration test~~ - [x] Closes celestiaorg#968

rahulghangas changed the title ~~Feat/emit typed event~~ feat: switch to emit typed event Nov 17, 2022

rahulghangas self-assigned this Nov 17, 2022

rahulghangas added C: Celestia app enhancement New feature or request labels Nov 17, 2022

rahulghangas changed the title ~~feat: switch to emit typed event~~ feat: switch to using typed events Nov 17, 2022

rahulghangas changed the title ~~feat: switch to using typed events~~ feat: use typed events Nov 17, 2022

vidhanarya reviewed Nov 18, 2022

View reviewed changes

x/blob/keeper/keeper.go Outdated Show resolved Hide resolved

vidhanarya reviewed Nov 18, 2022

View reviewed changes

rahulghangas marked this pull request as ready for review November 18, 2022 11:14

rahulghangas requested a review from evan-forbes as a code owner November 18, 2022 11:14

rahulghangas requested review from rootulp and rach-id November 18, 2022 11:14

rach-id previously requested changes Nov 18, 2022

View reviewed changes

proto/blob/events.proto Outdated Show resolved Hide resolved

x/blob/keeper/keeper.go Outdated Show resolved Hide resolved

rahulghangas requested a review from rach-id November 18, 2022 11:26

rach-id reviewed Nov 18, 2022

View reviewed changes

rootulp reviewed Nov 18, 2022

View reviewed changes

rahulghangas requested review from rach-id and rootulp November 25, 2022 11:49

rootulp reviewed Nov 25, 2022

View reviewed changes

proto/blob/events.proto Outdated Show resolved Hide resolved

rahulghangas added 7 commits November 29, 2022 19:06

feat: define payforblob event as protobuf message

f93b503

chore: generate file from protobuf

06d6631

chore: use EmitTypedEvent instead of EmitEvent

079e5aa

chore: rename PayForBlob to EventPayForBlob, key_size to blob_size

3054c72

chore: gofumpted

2b88a2a

chore: rename PayForBlob to EventPayForBlob, key_size to blob_size

08e5311

chore: add comment for EventPayForBlob

e679ee6

rahulghangas added 10 commits November 29, 2022 19:06

chore: generate files fromn protobuf

a8a86c8

chore: shift event struct creation to NewPayForBlobEvent

27ce0ab

chore: rename event.proto tp events.proto

85f7af1

chore: remove leftover proto generated file

049366e

chore: make EventTypePayForBlob dependent on derived value than being…

759b00b

… hardcoded

fix: return error instead of nil

718edf1

chore: formatting

60fb499

chore: rename to event.proto and corresponding proto gen'd file

3cc8d4b

chore: row formatting

e596e07

chore: add new event format to blob/README

3f5bd02

rahulghangas force-pushed the feat/emit-typed-event branch from 4789456 to 3f5bd02 Compare November 29, 2022 13:42

rahulghangas requested a review from rootulp November 29, 2022 13:52

rootulp reviewed Nov 29, 2022

View reviewed changes

rahulghangas added 2 commits December 8, 2022 15:11

fix: module name

4847206

fix: doc comment

e2cd3fd

evan-forbes approved these changes Dec 14, 2022

View reviewed changes

rahulghangas merged commit c16d6e7 into celestiaorg:main Dec 15, 2022

rahulghangas deleted the feat/emit-typed-event branch December 15, 2022 11:49

rahulghangas mentioned this pull request Dec 15, 2022

Add test for address validation for account emitted in event #1129

Closed

rootulp pushed a commit to rootulp/celestia-app that referenced this pull request Dec 19, 2022

feat: use typed events (celestiaorg#1030)

a8b7ab8

- [x] switch to typed event for `blob` module' - [ ] ~~Refactor integration test~~ - [x] Closes celestiaorg#968

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use typed events #1030

feat: use typed events #1030

rahulghangas commented Nov 17, 2022 •

edited

codecov-commenter commented Nov 17, 2022 •

edited

vidhanarya Nov 18, 2022

rahulghangas Nov 18, 2022

vidhanarya Nov 18, 2022

rahulghangas commented Nov 18, 2022

rach-id left a comment

rach-id Nov 18, 2022

evan-forbes Nov 18, 2022

rahulghangas Nov 22, 2022

rach-id Nov 25, 2022

rahulghangas Nov 29, 2022

rach-id Nov 29, 2022

rootulp Nov 29, 2022

rahulghangas Dec 8, 2022

rootulp left a comment

evan-forbes commented Nov 21, 2022

rahulghangas commented Nov 22, 2022

rootulp left a comment

rootulp Nov 29, 2022

evan-forbes left a comment

	types.NewPayForBlobEvent(sdk.AccAddress(msg.Signer).String(), msg.GetBlobSize()),
	types.NewPayForBlobEvent(msg.Signer, msg.GetBlobSize()),

	## Events

	- [`NewPayForBlobEvent`](https://github.com/celestiaorg/celestia-app/pull/213/files#diff-1ce55bda42cf160deca2e5ea1f4382b65f3b689c7e00c88085d7ce219e77303dR17-R21) is emitted with the signer's address and size of the message that is paid for.

feat: use typed events #1030

feat: use typed events #1030

Conversation

rahulghangas commented Nov 17, 2022 • edited

codecov-commenter commented Nov 17, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahulghangas commented Nov 18, 2022

rach-id 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

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

evan-forbes commented Nov 21, 2022

rahulghangas commented Nov 22, 2022

rootulp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

rahulghangas commented Nov 17, 2022 •

edited

codecov-commenter commented Nov 17, 2022 •

edited