-
Notifications
You must be signed in to change notification settings - Fork 608
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
x/superfluid: ensure events are emitted and tests added #2255
Changes from 11 commits
963b0f4
9abb176
cd82fe7
c0dd4b1
20cba08
3dbe40c
e8b4fca
f357e10
41f8b78
8390a5a
e72df0d
eddaf55
1e2b423
58fbe27
5178a31
d7102b8
f119784
9506eb1
f42e205
bc9cbf5
9c79c43
d3dd488
7ec606b
d808e28
ac87b0f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,6 +59,7 @@ func (suite *KeeperTestSuite) TestHandleSetSuperfluidAssetsProposal() { | |
assets []types.SuperfluidAsset | ||
expectedAssets []types.SuperfluidAsset | ||
expectErr bool | ||
expectedEvent string | ||
} | ||
testCases := []struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we able to / should we break down this actions struct by item? Maybe its not a big deal but just seeing a lump of five items together doesn't make the table driven test super easy to figure out what is getting tested. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, naming fields is preferable for readability IMO |
||
name string | ||
|
@@ -68,21 +69,21 @@ func (suite *KeeperTestSuite) TestHandleSetSuperfluidAssetsProposal() { | |
"happy path flow", | ||
[]Action{ | ||
{ | ||
true, []types.SuperfluidAsset{asset1}, []types.SuperfluidAsset{asset1}, false, | ||
true, []types.SuperfluidAsset{asset1}, []types.SuperfluidAsset{asset1}, false, types.TypeEvtSetSuperfluidAsset, | ||
}, | ||
{ | ||
false, []types.SuperfluidAsset{asset1}, []types.SuperfluidAsset{}, false, | ||
false, []types.SuperfluidAsset{asset1}, []types.SuperfluidAsset{}, false, types.TypeEvtRemoveSuperfluidAsset, | ||
}, | ||
}, | ||
}, | ||
{ | ||
"token does not exist", | ||
[]Action{ | ||
{ | ||
true, []types.SuperfluidAsset{asset1}, []types.SuperfluidAsset{asset1}, false, | ||
true, []types.SuperfluidAsset{asset1}, []types.SuperfluidAsset{asset1}, false, types.TypeEvtSetSuperfluidAsset, | ||
}, | ||
{ | ||
false, []types.SuperfluidAsset{asset2}, []types.SuperfluidAsset{asset1}, true, | ||
false, []types.SuperfluidAsset{asset2}, []types.SuperfluidAsset{asset1}, true, types.TypeEvtRemoveSuperfluidAsset, | ||
}, | ||
}, | ||
}, | ||
|
@@ -132,6 +133,7 @@ func (suite *KeeperTestSuite) TestHandleSetSuperfluidAssetsProposal() { | |
suite.Require().Error(err) | ||
} else { | ||
suite.Require().NoError(err) | ||
assertEventEmitted(suite, suite.ctx, action.expectedEvent, 1) | ||
} | ||
|
||
// check assets individually | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,133 @@ | ||
package events | ||
|
||
import ( | ||
"fmt" | ||
|
||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
|
||
"github.com/osmosis-labs/osmosis/v10/x/superfluid/types" | ||
) | ||
|
||
func EmitSetSuperfluidAssetEvent(ctx sdk.Context, denom string, assetType types.SuperfluidAssetType) { | ||
if ctx.EventManager() == nil { | ||
return | ||
} | ||
|
||
ctx.EventManager().EmitEvents(sdk.Events{ | ||
newSetSuperfluidAssetEvent(denom, assetType), | ||
}) | ||
} | ||
|
||
func newSetSuperfluidAssetEvent(denom string, assetType types.SuperfluidAssetType) sdk.Event { | ||
return sdk.NewEvent( | ||
types.TypeEvtSetSuperfluidAsset, | ||
sdk.NewAttribute(types.AttributeDenom, denom), | ||
sdk.NewAttribute(types.AttributeSuperfluidAssetType, assetType.String()), | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we are going over all the module events, what are the thoughts on using It'll definitely reduce the code size and gives a better code clarity. For instance the above code could be rewritten with an event.proto file as;
ps: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think that's a great idea. However, I'm guessing we would make the Also, I think we would have to make the event constructors exported. For example, To have:
Just to avoid any confusion - we should pursue this separately (not in this PR) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we reusing events? I would only use constructors and separate out the event logic if we use the same ones in multiple places, otherwise it would be overkill. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are in 0 rush to switch to typed events. I also agree that constructors seem overkill unless reuse |
||
} | ||
|
||
func EmitRemoveSuperfluidAsset(ctx sdk.Context, denom string) { | ||
if ctx.EventManager() == nil { | ||
return | ||
} | ||
|
||
ctx.EventManager().EmitEvents(sdk.Events{ | ||
newRemoveSuperfluidAssetEvent(denom), | ||
}) | ||
} | ||
|
||
func newRemoveSuperfluidAssetEvent(denom string) sdk.Event { | ||
return sdk.NewEvent( | ||
types.TypeEvtRemoveSuperfluidAsset, | ||
sdk.NewAttribute(types.AttributeDenom, denom), | ||
) | ||
} | ||
|
||
func EmitSuperfluidDelegateEvent(ctx sdk.Context, lockId uint64, valAddress string) { | ||
p0mvn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if ctx.EventManager() == nil { | ||
return | ||
} | ||
|
||
ctx.EventManager().EmitEvents(sdk.Events{ | ||
newSuperfluidDelegateEvent(lockId, valAddress), | ||
}) | ||
} | ||
|
||
func newSuperfluidDelegateEvent(lockId uint64, valAddress string) sdk.Event { | ||
return sdk.NewEvent( | ||
types.TypeEvtSuperfluidDelegate, | ||
sdk.NewAttribute(types.AttributeLockId, fmt.Sprintf("%d", lockId)), | ||
hieuvubk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
sdk.NewAttribute(types.AttributeValidator, valAddress), | ||
) | ||
} | ||
|
||
func EmitSuperfluidIncreaseDelegationEvent(ctx sdk.Context, lockId uint64, amount sdk.Coins) { | ||
if ctx.EventManager() == nil { | ||
return | ||
} | ||
|
||
ctx.EventManager().EmitEvents(sdk.Events{ | ||
newSuperfluidIncreaseDelegationEvent(lockId, amount), | ||
}) | ||
} | ||
|
||
func newSuperfluidIncreaseDelegationEvent(lockId uint64, amount sdk.Coins) sdk.Event { | ||
return sdk.NewEvent( | ||
types.TypeEvtSuperfluidIncreaseDelegation, | ||
sdk.NewAttribute(types.AttributeLockId, fmt.Sprintf("%d", lockId)), | ||
sdk.NewAttribute(types.AttributeAmount, amount.String()), | ||
) | ||
} | ||
|
||
func EmitSuperfluidUndelegateEvent(ctx sdk.Context, lockId uint64) { | ||
if ctx.EventManager() == nil { | ||
return | ||
} | ||
|
||
ctx.EventManager().EmitEvents(sdk.Events{ | ||
newSuperfluidUndelegateEvent(lockId), | ||
}) | ||
} | ||
|
||
func newSuperfluidUndelegateEvent(lockId uint64) sdk.Event { | ||
return sdk.NewEvent( | ||
types.TypeEvtSuperfluidUndelegate, | ||
sdk.NewAttribute(types.AttributeLockId, fmt.Sprintf("%d", lockId)), | ||
) | ||
} | ||
|
||
func EmitSuperfluidUnbondLockEvent(ctx sdk.Context, lockId uint64) { | ||
if ctx.EventManager() == nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh thank you. If so, I'll remove them. |
||
return | ||
} | ||
|
||
ctx.EventManager().EmitEvents(sdk.Events{ | ||
newSuperfluidUnbondLockEvent(lockId), | ||
}) | ||
} | ||
|
||
func newSuperfluidUnbondLockEvent(lockId uint64) sdk.Event { | ||
return sdk.NewEvent( | ||
types.TypeEvtSuperfluidUnbondLock, | ||
sdk.NewAttribute(types.AttributeLockId, fmt.Sprintf("%d", lockId)), | ||
) | ||
} | ||
|
||
func EmitUnpoolIdEvent(ctx sdk.Context, sender string, lpShareDenom string, allExitedLockIDsSerialized []byte) { | ||
if ctx.EventManager() == nil { | ||
return | ||
} | ||
|
||
ctx.EventManager().EmitEvents(sdk.Events{ | ||
newUnpoolIdEvent(sender, lpShareDenom, allExitedLockIDsSerialized), | ||
}) | ||
} | ||
|
||
func newUnpoolIdEvent(sender string, lpShareDenom string, allExitedLockIDsSerialized []byte) sdk.Event { | ||
return sdk.NewEvent( | ||
types.TypeEvtUnpoolId, | ||
sdk.NewAttribute(sdk.AttributeKeySender, sender), | ||
sdk.NewAttribute(types.AttributeDenom, lpShareDenom), | ||
sdk.NewAttribute(types.AttributeNewLockIds, string(allExitedLockIDsSerialized)), | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the motivation for this, but I can see this going out of date and being difficult to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might still want to have it since these docs get auto-uploaded to the docs website. Currently, there is no info about events but it could be useful for integrators. WDYT?