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: synch dynamic fees only when `InputFee` is active #1108

Merged
merged 11 commits into from Mar 8, 2019

Conversation

@j-a-m-l
Copy link
Contributor

commented Feb 28, 2019

Proposed changes

These changes avoid making requests at the beginning to fetch the fee statistics of every network, and then, periodically to have them updated.
Now, only the current network is checked, but just in the case that InputFee component is active, because they are only used there.

This also improves the Synchronizer service, so, from now on, we could disable completely some actions until needed.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Refactoring (improve a current implementation without adding a new feature or fixing a bug)
  • Test (adding missing tests or fixing existing tests)

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

@alexbarnsley @luciorubeens - please review this in the next few days. Be sure to explicitly select labels so I know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

@codecov-io

This comment has been minimized.

Copy link

commented Feb 28, 2019

Codecov Report

Merging #1108 into develop will increase coverage by 0.02%.
The diff coverage is 36.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #1108      +/-   ##
==========================================
+ Coverage    40.37%   40.4%   +0.02%     
==========================================
  Files          207     207              
  Lines         5280    5250      -30     
  Branches      1030    1028       -2     
==========================================
- Hits          2132    2121      -11     
+ Misses        3034    3019      -15     
+ Partials       114     110       -4
Impacted Files Coverage Δ
src/renderer/store/index.js 87.5% <ø> (ø) ⬆️
src/renderer/store/modules/transaction.js 52.11% <ø> (ø) ⬆️
src/renderer/services/client.js 71.57% <0%> (+0.94%) ⬆️
src/renderer/store/modules/network.js 46.34% <0%> (+11.79%) ⬆️
src/renderer/store/modules/peer.js 69.3% <100%> (ø) ⬆️
src/renderer/store/modules/session.js 35.51% <100%> (ø) ⬆️
src/renderer/services/synchronizer.js 21.21% <15.78%> (+0.89%) ⬆️
src/renderer/components/Input/InputFee.vue 69.62% <57.69%> (-2.18%) ⬇️
.../components/Wallet/WalletSidebar/WalletSidebar.vue 59.7% <0%> (-2.8%) ⬇️
src/renderer/mixins/currency.js 88.23% <0%> (-1.24%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3bd276...4d3a628. Read the comment docs.

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

Your pull request needs some changes. Please wait for a comment from one of our developers for more information.

@j-a-m-l

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@luciorubeens I've updated the minimum fee part. It was intended to support getting the minimum fee from the statistics, but, since we aren't doing it, it wasn't necessary at all.

About the other change, the idea was keeping the fees as updated as possible only when the InputFee is visible (when the component is destroyed, the synchronizer action is paused again). Possibly the majority of users wouldn't use it, but I've added that to not leave any loose ends.

@luciorubeens luciorubeens merged commit a0c1218 into develop Mar 8, 2019

1 check passed

ci/circleci: test-node-11 Your tests passed on CircleCI!
Details

@ArkEcosystemBot ArkEcosystemBot deleted the fetch-fees-only-when-necessary branch Mar 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.