-
Notifications
You must be signed in to change notification settings - Fork 624
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
refactor: change sdk.Events usage to []abci.Event #3980
Conversation
@@ -163,7 +163,7 @@ func (suite *KeeperTestSuite) TestDistributeFeeEvent() { | |||
sdk.NewAttribute(types.AttributeKeyReceiver, suite.chainA.SenderAccount.GetAddress().String()), | |||
sdk.NewAttribute(types.AttributeKeyFee, defaultTimeoutFee.String()), | |||
), | |||
} | |||
}.ToABCIEvents() |
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.
for convenience I left as is and called ToABCIEvents()
since sdk.Events
is just a wrapper around []abci.Event
@@ -6,7 +6,7 @@ import ( | |||
|
|||
"github.com/stretchr/testify/suite" | |||
|
|||
sdk "github.com/cosmos/cosmos-sdk/types" | |||
abci "github.com/cometbft/cometbft/abci/types" |
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.
seems like we have slight inconsistency between abci
alias and abcitypes
alias. I think I prefer abci
just because there is no go file at that level https://github.com/cometbft/cometbft/tree/main/abci, but I don't have a strong preference. We can make consistent in a followup
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 like abci
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.
Nice, looks pretty clean to me! I think we're using ParsePacketFromEvents()
in e2e as well and CI builds look like they're failing. Should be an easy enough fix I think :)
@@ -6,7 +6,7 @@ import ( | |||
|
|||
"github.com/stretchr/testify/suite" | |||
|
|||
sdk "github.com/cosmos/cosmos-sdk/types" | |||
abci "github.com/cometbft/cometbft/abci/types" |
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 like abci
oh yea! My bad. Fixed :) |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3980 +/- ##
=======================================
Coverage 78.84% 78.84%
=======================================
Files 187 187
Lines 12950 12950
=======================================
Hits 10211 10211
Misses 2311 2311
Partials 428 428
|
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.
nice! it'll definitely be good to cut down the file changes for sdk 50!
@charleenfei I agree. Definitely think this is a helpful change. |
Description
Note: When changing from
*sdk.Result
to*abci.ExecTxResult
,GetEvents()
would have begun returning[]abci.Event
, but I decided to pull out these changes anyways to reduce the burden of "why is this changing" on reviewers when reviewing the update to SDK v0.50Let me know if you think this deserves a changelog even if no code changes would need to be made by downstream users since they would update to this change at the same time as the SDK bump
closes: #3979
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
).godoc
comments.Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.