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

(rpc-test) accurately account for fee without rounding error #3303

Merged
merged 2 commits into from Jun 22, 2018

Conversation

@LarryRuane
Copy link
Contributor

LarryRuane commented May 24, 2018

Fix for #2807, this test compares balances after doing key exports and imports, and expects these balances to be equal. But they are not exactly equal due to transaction fees, so the test makes them "equal" by rounding a value that has had fees taken out up to the nearest hundredth of a unit (which is much more than the default fee). This obviously is somewhat sloppy. It also converts a balance to float, which really should never be used due to loss of precision (use Decimal instead).

This change makes the test accurately account for the fee using precise comparisons, and removes the use of float. This test doesn't depend on the default fee (0.0001) but instead sets the fee (to that value). This way, if the default fee changes in the future, this test will continue to run. While testing these changes, I set the fee to various values (up to the max, 0.0190), and the test still passes.

@LarryRuane LarryRuane requested review from str4d and Eirik0 May 24, 2018

@Eirik0
Copy link
Contributor

Eirik0 left a comment

ACK.

@Eirik0

Eirik0 approved these changes May 25, 2018

@Eirik0 Eirik0 requested a review from bitcartel Jun 11, 2018

@LarryRuane LarryRuane requested a review from mdr0id Jun 18, 2018

@bitcartel
Copy link
Contributor

bitcartel left a comment

ACK, tested, works as advertised!

@bitcartel bitcartel added this to the v1.1.2 milestone Jun 22, 2018

@bitcartel bitcartel added the testing label Jun 22, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jun 22, 2018

Note that this PR did not show up zcashd team's agile project board, it slipped through the cracks.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jun 22, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 22, 2018

📌 Commit fff0316 has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 22, 2018

⌛️ Testing commit fff0316 with merge f76d813...

zkbot added a commit that referenced this pull request Jun 22, 2018

Auto merge of #3303 - LarryRuane:2807-rpc-test-round-fees, r=bitcartel
(rpc-test) accurately account for fee without rounding error

Fix for #2807, this test compares balances after doing key exports and imports, and expects these balances to be equal. But they are not exactly equal due to transaction fees, so the test makes them "equal" by rounding a value that has had fees taken out up to the nearest hundredth of a unit (which is much more than the default fee). This obviously is somewhat sloppy. It also converts a balance to float, which really should never be used due to loss of precision (use Decimal instead).

This change makes the test accurately account for the fee using precise comparisons, and removes the use of float. This test doesn't depend on the default fee (0.0001) but instead sets the fee (to that value). This way, if the default fee changes in the future, this test will continue to run. While testing these changes, I set the fee to various values (up to the max, 0.0190), and the test still passes.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 22, 2018

💔 Test failed - pr-merge

@str4d str4d added this to In Review in Zcashd Team Jun 22, 2018

def z_getbalance(node, zaddr):
bal = node.z_getbalance(zaddr)
# Ignore fees for sake of comparison
round_balance = math.ceil(bal*100)/100

This comment has been minimized.

@str4d

str4d Jun 22, 2018

Contributor

Removal of this causes the math module to become unused, triggering a PyFlakes warning and preventing merge.

This comment has been minimized.

@bitcartel

bitcartel Jun 22, 2018

Contributor

Fixing now.

@str4d str4d moved this from In Review to In Progress in Zcashd Team Jun 22, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jun 22, 2018

@zkbot retry

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jun 22, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 22, 2018

⌛️ Trying commit a4ff089 with merge b07fdd3...

zkbot added a commit that referenced this pull request Jun 22, 2018

Auto merge of #3303 - LarryRuane:2807-rpc-test-round-fees, r=<try>
(rpc-test) accurately account for fee without rounding error

Fix for #2807, this test compares balances after doing key exports and imports, and expects these balances to be equal. But they are not exactly equal due to transaction fees, so the test makes them "equal" by rounding a value that has had fees taken out up to the nearest hundredth of a unit (which is much more than the default fee). This obviously is somewhat sloppy. It also converts a balance to float, which really should never be used due to loss of precision (use Decimal instead).

This change makes the test accurately account for the fee using precise comparisons, and removes the use of float. This test doesn't depend on the default fee (0.0001) but instead sets the fee (to that value). This way, if the default fee changes in the future, this test will continue to run. While testing these changes, I set the fee to various values (up to the max, 0.0190), and the test still passes.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 22, 2018

☀️ Test successful - pr-try
State: approved= try=True

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jun 22, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 22, 2018

📌 Commit a4ff089 has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 22, 2018

⌛️ Testing commit a4ff089 with merge f649bb3...

zkbot added a commit that referenced this pull request Jun 22, 2018

Auto merge of #3303 - LarryRuane:2807-rpc-test-round-fees, r=bitcartel
(rpc-test) accurately account for fee without rounding error

Fix for #2807, this test compares balances after doing key exports and imports, and expects these balances to be equal. But they are not exactly equal due to transaction fees, so the test makes them "equal" by rounding a value that has had fees taken out up to the nearest hundredth of a unit (which is much more than the default fee). This obviously is somewhat sloppy. It also converts a balance to float, which really should never be used due to loss of precision (use Decimal instead).

This change makes the test accurately account for the fee using precise comparisons, and removes the use of float. This test doesn't depend on the default fee (0.0001) but instead sets the fee (to that value). This way, if the default fee changes in the future, this test will continue to run. While testing these changes, I set the fee to various values (up to the max, 0.0190), and the test still passes.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 22, 2018

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing f649bb3 to master...

@zkbot zkbot merged commit a4ff089 into zcash:master Jun 22, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from In Progress to Released (Merged in Master) Jun 22, 2018

@LarryRuane LarryRuane deleted the LarryRuane:2807-rpc-test-round-fees branch Jun 23, 2018

@LarryRuane LarryRuane self-assigned this Jul 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment