Skip to content

Filter fee events#1188

Merged
chasefleming merged 12 commits into
masterfrom
filter-fee-events
Sep 20, 2023
Merged

Filter fee events#1188
chasefleming merged 12 commits into
masterfrom
filter-fee-events

Conversation

@bthaile

@bthaile bthaile commented Sep 6, 2023

Copy link
Copy Markdown
Contributor

Closes: #1151

Description

  • filter out "FeeEvent" and "FlowToken" events by default.
  • Add new included fee-events flag so users can get all events including the fee events.
  • Added two new tests to test filtering events
  • Updated transactions get command include flags help

For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@codecov-commenter

codecov-commenter commented Sep 7, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.25% 🎉

Comparison is base (3148a57) 38.40% compared to head (aecdb6c) 38.65%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1188      +/-   ##
==========================================
+ Coverage   38.40%   38.65%   +0.25%     
==========================================
  Files          38       38              
  Lines        1940     1948       +8     
==========================================
+ Hits          745      753       +8     
  Misses       1107     1107              
  Partials       88       88              
Flag Coverage Δ
unittests 38.65% <100.00%> (+0.25%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
internal/transactions/get.go 81.81% <ø> (ø)
internal/transactions/transactions.go 62.74% <100.00%> (+3.17%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bjartek

bjartek commented Sep 7, 2023

Copy link
Copy Markdown
Collaborator

Will this not remove all FlowToken events?
The fee events are always the 3 latest in a tx. Just remove them?
So the withdraw/deposit/fee
I think fee and effort should be returned in TransactionResult personally. Those 3 events are different the the others cause you did not emit them. The system did.

@bthaile

bthaile commented Sep 7, 2023

Copy link
Copy Markdown
Contributor Author

Will this not remove all FlowToken events? The fee events are always the 3 latest in a tx. Just remove them? So the withdraw/deposit/fee I think fee and effort should be returned in TransactionResult personally. Those 3 events are different the the others cause you did not emit them. The system did.

I was going off the github issue linked. Spot checking some transactions, I see what you are saying the last three events are what need to be filtered out. I'll update the PR.
Thanks

@janezpodhostnik janezpodhostnik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In certain setups (fees are set to 0) there will be no fee events. I would make the check more complex:

If a FeesDeducted event is present (should be the last one) then the last 3 events are fee events.

Comment thread internal/transactions/transactions.go Outdated
Comment thread internal/transactions/transactions.go Outdated
bthaile and others added 4 commits September 18, 2023 14:23
Co-authored-by: Janez Podhostnik <67895329+janezpodhostnik@users.noreply.github.com>
@chasefleming

Copy link
Copy Markdown
Member

@bthaile Can we make sure we update the docs wherever necessary with this new flag?

@bthaile

bthaile commented Sep 20, 2023

Copy link
Copy Markdown
Contributor Author

@bthaile Can we make sure we update the docs wherever necessary with this new flag?

Updated here, internal/transactions/get.go in the PR. Are their other docs?

good call, I can update developer docs, here https://developers.flow.com/tools/flow-cli/transactions/get-transactions

created docs github issue
onflow/docs#358

@chasefleming

Copy link
Copy Markdown
Member

Thanks @bthaile !

@chasefleming chasefleming merged commit 7c53925 into master Sep 20, 2023
@chasefleming chasefleming deleted the filter-fee-events branch September 20, 2023 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve events display

5 participants