Skip to content
This repository has been archived by the owner on Jul 27, 2022. It is now read-only.

Problem: incorrect fee display in history (#846) #969

Merged
merged 1 commit into from
Jan 30, 2020
Merged

Problem: incorrect fee display in history (#846) #969

merged 1 commit into from
Jan 30, 2020

Conversation

tomtau
Copy link
Contributor

@tomtau tomtau commented Jan 30, 2020

Solution:

  • removed the "balance change" output custom fee calculation / deduction
  • added paid fee to the balance change that's directly taken from events / attributes

Note: transaction details was moved to a separate issue: #968

@tomtau
Copy link
Contributor Author

tomtau commented Jan 30, 2020

bors try

bors bot added a commit that referenced this pull request Jan 30, 2020
@bors
Try #969:
de20922
@bors
Copy link
Contributor

bors bot commented Jan 30, 2020

try

Build failed

calvinaco
calvinaco reviewed Jan 30, 2020
View reviewed changes
Copy link
Collaborator

@calvinaco calvinaco left a comment

Choose a reason for hiding this comment

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

The code looks good to me. Just a small changes to code comment.

However, I want to understand more about the background, isn't the fee paid should be the same as sum of input minus sum of outputs?

client-common/src/tendermint/types/block_results.rs Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 30, 2020 •
edited
Loading

Codecov Report

Merging #969 into master will increase coverage by 0.04%.
The diff coverage is 71.13%.

@@            Coverage Diff             @@
##           master     #969      +/-   ##
==========================================
+ Coverage   67.29%   67.33%   +0.04%     
==========================================
  Files         137      137              
  Lines       18051    18092      +41     
==========================================
+ Hits        12147    12182      +35     
- Misses       5904     5910       +6
Impacted Files Coverage Δ
chain-core/src/tx/fee/mod.rs 87.25% <0%> (-0.93%) ⬇️
client-core/src/wallet/default_wallet_client.rs 41.7% <0%> (-0.69%) ⬇️
client-cli/src/command.rs 0% <0%> (ø) ⬆️
client-core/src/types/transaction_change.rs 85.18% <100%> (+1.74%) ⬆️
client-core/src/service/wallet_state_service.rs 90.21% <100%> (+0.02%) ⬆️
client-core/src/wallet/syncer_logic.rs 95% <100%> (+1.8%) ⬆️
client-core/src/wallet/syncer.rs 61.12% <75%> (ø) ⬆️
...lient-common/src/tendermint/types/block_results.rs 85.41% <84.21%> (-0.27%) ⬇️
... and 1 more

@tomtau
Copy link
Contributor Author

tomtau commented Jan 30, 2020

The code looks good to me. Just a small changes to code comment.

However, I want to understand more about the background, isn't the fee paid should be the same as sum of input minus sum of outputs?

@calvinaco see #846
it depends on the transaction type (in some, there are no inputs or no outputs) -- I think the old display code was done when there was only transfertx.
One other thing is that for incoming tx, one may not know what the total amount was if the wallet didn't know the inputs.

calvinaco
calvinaco approved these changes Jan 30, 2020
View reviewed changes
Copy link
Collaborator

@calvinaco calvinaco left a comment

Choose a reason for hiding this comment

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

Ok I see!

@tomtau
Copy link
Contributor Author

tomtau commented Jan 30, 2020

bors r+

bors bot added a commit that referenced this pull request Jan 30, 2020
@bors @tomtau
Merge #969
028d8be 
969: Problem: incorrect fee display in history (#846) r=tomtau a=tomtau

Solution:
- removed the "balance change" output custom fee calculation / deduction
- added paid fee to the balance change that's directly taken from events / attributes

Note: transaction details was moved to a separate issue: #968

Co-authored-by: Tomas Tauber <2410580+tomtau@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Jan 30, 2020

Build failed

@tomtau
Copy link
Contributor Author

tomtau commented Jan 30, 2020

bors try

bors bot added a commit that referenced this pull request Jan 30, 2020
@bors
Try #969:
6f51071
@bors
Copy link
Contributor

bors bot commented Jan 30, 2020

try

Build failed

@tomtau
Problem: incorrect fee display in history (fix crypto-com#846)
5348011 
Solution:
- removed the "balance change" output custom fee calculation / deduction
- added paid fee to the balance change that's directly taken from events / attributes

Note: transaction details was moved to a separate issue: crypto-com#968
@tomtau
Copy link
Contributor Author

tomtau commented Jan 30, 2020

bors r+

bors bot added a commit that referenced this pull request Jan 30, 2020
@bors @tomtau
Merge #969
e7356b8 
969: Problem: incorrect fee display in history (#846) r=tomtau a=tomtau

Solution:
- removed the "balance change" output custom fee calculation / deduction
- added paid fee to the balance change that's directly taken from events / attributes

Note: transaction details was moved to a separate issue: #968

Co-authored-by: Tomas Tauber <2410580+tomtau@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Jan 30, 2020

Build failed

@tomtau
Copy link
Contributor Author

tomtau commented Jan 30, 2020

bors retry

bors bot added a commit that referenced this pull request Jan 30, 2020
@bors @tomtau
Merge #969
2779654 
969: Problem: incorrect fee display in history (#846) r=tomtau a=tomtau

Solution:
- removed the "balance change" output custom fee calculation / deduction
- added paid fee to the balance change that's directly taken from events / attributes

Note: transaction details was moved to a separate issue: #968

Co-authored-by: Tomas Tauber <2410580+tomtau@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Jan 30, 2020

Build succeeded

@bors bors bot merged commit 5348011 into crypto-com:master Jan 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@calvinaco calvinaco calvinaco approved these changes

@yihuang yihuang yihuang approved these changes

@devashishdxt devashishdxt Awaiting requested review from devashishdxt devashishdxt is a code owner

@leejw51crypto leejw51crypto Awaiting requested review from leejw51crypto

@linfeng-crypto linfeng-crypto Awaiting requested review from linfeng-crypto

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tomtau @yihuang @calvinaco