Skip to content
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(x/bank): remove message events including sender attribute whose information is already present in the relevant events (backport cosmos/cosmos#17273) #1066

Merged
merged 8 commits into from
Aug 17, 2023

Conversation

0Tech
Copy link
Collaborator

@0Tech 0Tech commented Aug 4, 2023

Description

This patch would remove duplicate message events in x/bank keeper.

You may refer to the PR in the upstream.

Checklist:

  • I followed the contributing guidelines and code of conduct.
  • I have added a relevant changelog to CHANGELOG.md
  • I have added tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have updated API documentation client/docs/swagger-ui/swagger.yaml

@0Tech 0Tech added the A: improvement Changes in existing functionality label Aug 4, 2023
@0Tech 0Tech self-assigned this Aug 4, 2023
@CLAassistant
Copy link

CLAassistant commented Aug 4, 2023

CLA assistant check
All committers have signed the CLA.

@0Tech

This comment was marked as resolved.

CHANGELOG.md Outdated Show resolved Hide resolved
@0Tech 0Tech force-pushed the remove-keeper-sender branch from 39d02b3 to 62c6a4d Compare August 10, 2023 07:04
@0Tech 0Tech force-pushed the remove-keeper-sender branch from f24d77a to 29bf70d Compare August 11, 2023 00:24
@0Tech 0Tech force-pushed the remove-keeper-sender branch from 353de61 to 2afab69 Compare August 11, 2023 01:42
@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Merging #1066 (5a0edf5) into release/v0.47.x (92d75cf) will increase coverage by 0.10%.
Report is 4 commits behind head on release/v0.47.x.
The diff coverage is 85.18%.

Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                 @@
##           release/v0.47.x    #1066      +/-   ##
===================================================
+ Coverage            62.04%   62.14%   +0.10%     
===================================================
  Files                  655      655              
  Lines                78983    78866     -117     
===================================================
+ Hits                 49004    49011       +7     
+ Misses               27293    27169     -124     
  Partials              2686     2686              
Files Changed Coverage Δ
x/auth/vesting/msg_server.go 67.27% <ø> (-3.70%) ⬇️
x/bank/keeper/msg_server.go 3.84% <ø> (+0.81%) ⬆️
x/bank/types/events.go 0.00% <ø> (ø)
x/crisis/keeper/msg_server.go 0.00% <ø> (ø)
x/distribution/keeper/msg_server.go 2.66% <ø> (+0.77%) ⬆️
x/evidence/keeper/msg_server.go 16.66% <ø> (+6.66%) ⬆️
x/gov/keeper/msg_server.go 2.24% <0.00%> (+0.59%) ⬆️
x/slashing/keeper/msg_server.go 14.28% <ø> (+5.19%) ⬆️
x/staking/keeper/msg_server.go 0.74% <ø> (+0.06%) ⬆️
baseapp/baseapp.go 79.13% <100.00%> (+0.46%) ⬆️
... and 3 more

... and 2 files with indirect coverage changes

@0Tech 0Tech marked this pull request as ready for review August 11, 2023 02:56
@0Tech 0Tech requested review from zemyblue and jaeseung-bae August 11, 2023 03:01
Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the AttributeKeySender of bank module is no need.

AttributeKeySender = "sender"

@zemyblue zemyblue added this to the v0.47.3 milestone Aug 16, 2023
@0Tech
Copy link
Collaborator Author

0Tech commented Aug 16, 2023

I think the AttributeKeySender of bank module is no need.

AttributeKeySender = "sender"

The attribute is still in use, e.g. in transfer event.

Copy link
Collaborator Author

@0Tech 0Tech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the unit test.

x/authz/keeper/keeper_test.go Outdated Show resolved Hide resolved
x/authz/keeper/keeper_test.go Outdated Show resolved Hide resolved
@0Tech 0Tech requested review from zemyblue and jaeseung-bae August 16, 2023 06:56
@zemyblue
Copy link
Member

I think the AttributeKeySender of bank module is no need.

AttributeKeySender = "sender"

The attribute is still in use, e.g. in transfer event.

But I think it can be changed sdk.AttributeKeySender like

sdk.NewAttribute(sdk.AttributeKeyAmount, amt.String()),
.

@0Tech 0Tech merged commit 3d482e2 into Finschia:release/v0.47.x Aug 17, 2023
@0Tech 0Tech deleted the remove-keeper-sender branch August 17, 2023 04:32
0Tech added a commit to 0Tech/finschia-sdk that referenced this pull request Aug 24, 2023
…whose information is already present in the relevant events (backport cosmos/cosmos#17273) (Finschia#1066)

* refactor(x/bank): remove message events including `sender` attribute whose information is already present in the relevant events (#17273)

* Update CHANGELOG.md

* Update x/foundation tests

* Update CHANGELOG.md

* Update x/foundation tests

* Update other tests

* Apply suggestions from code review

Update x/authz test

* Use sdk.AttributeKeySender in x/bank type
0Tech added a commit that referenced this pull request Aug 24, 2023
…whose information is already present in the relevant events (backport #1066) (#1093)

* refactor(x/bank): remove message events including `sender` attribute whose information is already present in the relevant events (backport cosmos/cosmos#17273) (#1066)

* refactor(x/bank): remove message events including `sender` attribute whose information is already present in the relevant events (#17273)

* Update CHANGELOG.md

* Update x/foundation tests

* Update CHANGELOG.md

* Update x/foundation tests

* Update other tests

* Apply suggestions from code review

Update x/authz test

* Use sdk.AttributeKeySender in x/bank type

* Update CHANGELOG.md

* Update unit test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: improvement Changes in existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants