-
Notifications
You must be signed in to change notification settings - Fork 693
Closed
Labels
C:x/superfluidGood first issueT:bug 🐛Something isn't workingSomething isn't workingT:storyA story belongs to an epicA story belongs to an epicT:tests
Description
Background
There are a few issues with how we emit superfluid events.
First, some of them are not being emitted on success:
osmosis/x/superfluid/keeper/msg_server.go
Lines 49 to 55 in 979cbd4
| if err != nil { | |
| ctx.EventManager().EmitEvent(sdk.NewEvent( | |
| types.TypeEvtSuperfluidDelegate, | |
| sdk.NewAttribute(types.AttributeLockId, fmt.Sprintf("%d", msg.LockId)), | |
| sdk.NewAttribute(types.AttributeValidator, msg.ValAddr), | |
| )) | |
| } |
The conditional should be the opposite of what it is now.
Second, there are no tests for any superfluid events so we cannot be confident if the rest function as expected.
Third, there is no documentation about what events are for and which attributes they should contain.
Suggested Design
Follow a similar change in x/gamm: #1942
To limit the scope and ease the review, I suggest associating each PR with the following tasks:
- Task 1 - Core logic + Test
TypeEvtSetSuperfluidAsset
- Create an internal
x/superfluid/internal/eventspackage that contains core logic for emitting events - Keep the event names in
typessince they might be used by integrators - Follow this PR for an example in GAMM
- Test
TypeEvtSetSuperfluidAsset- test attributes in
x/superfluid/internal/events - test that the event is emitted in the right location
- Add docs to the README about what the event is for and which attributes it contains
- test attributes in
- Task 2 - Test
TypeEvtRemoveSuperfluidAsset
TestTypeEvtRemoveSuperfluidAsset- test attributes in
x/superfluid/internal/events - test that the event is emitted in the right location
- Add docs to the README about what the event is for and which attributes it contains
- test attributes in
- Task 3 - Test
TypeEvtSuperfluidDelegate
TestTypeEvtSuperfluidDelegate- test attributes in
x/superfluid/internal/events - test that the event is emitted in the right location
- Add docs to the README about what the event is for and which attributes it contains
- test attributes in
- Task 4 - Test
TypeEvtSuperfluidIncreaseDelegation
TestTypeEvtSuperfluidIncreaseDelegation- test attributes in
x/superfluid/internal/events - test that the event is emitted in the right location
- Add docs to the README about what the event is for and which attributes it contains
- test attributes in
- Task 5 - Test
TypeEvtSuperfluidUndelegate
TestTypeEvtSuperfluidUndelegate- test attributes in
x/superfluid/internal/events - test that the event is emitted in the right location
- Add docs to the README about what the event is for and which attributes it contains
- test attributes in
- Task 6 - Test
TypeEvtSuperfluidUnbondLock
TestTypeEvtSuperfluidUnbondLock- test attributes in
x/superfluid/internal/events - test that the event is emitted in the right location
- Add docs to the README about what the event is for and which attributes it contains
- test attributes in
- Task 7 - Test
TypeEvtUnpoolId
TestTypeEvtUnpoolId- test attributes in
x/superfluid/internal/events - test that the event is emitted in the right location
- Add docs to the README about what the event is for and which attributes it contains
- test attributes in
Acceptance Criteria
- superfluid events are emitted correctly in the right locations
- tests added for attributes
- tests added for verifying that events are emitted in the correct locations
- every event is documented in superfluid README
- what the events is for / when it is emitted
- what attributes it contains
- final design is consistent with x/gamm: x/gamm: test that swap and LP events are emitted #1942
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
C:x/superfluidGood first issueT:bug 🐛Something isn't workingSomething isn't workingT:storyA story belongs to an epicA story belongs to an epicT:tests
Type
Projects
Status
Done ✅