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

Improve fee and change logic #565

Closed
wants to merge 6 commits into from

Conversation

AloeareV
Copy link
Contributor

@AloeareV AloeareV commented Oct 4, 2023

Still in progress...some tests are now failing.

@AloeareV AloeareV force-pushed the improve_fee/change_logic branch from 8a542ac to 61b815e Compare October 6, 2023 20:17
@AloeareV AloeareV force-pushed the improve_fee/change_logic branch from 61b815e to aecbf8a Compare October 9, 2023 19:02
@AloeareV AloeareV marked this pull request as ready for review October 9, 2023 19:06
zingolib/src/wallet/data.rs Show resolved Hide resolved
zingocli/tests/integration_tests.rs Show resolved Hide resolved
zingolib/src/lightclient.rs Show resolved Hide resolved
zingolib/src/wallet/keys/unified.rs Outdated Show resolved Hide resolved
zingolib/src/wallet/transactions.rs Show resolved Hide resolved
zingolib/src/wallet/data.rs Show resolved Hide resolved
Copy link
Contributor

@fluidvanadium fluidvanadium left a comment

Choose a reason for hiding this comment

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

darn, i wish i could have beaten you to a failing test, but this should fix

Copy link
Contributor

@fluidvanadium fluidvanadium left a comment

Choose a reason for hiding this comment

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

wait hold on, i discovered something with a test that doesnt add up to my understanding!

@fluidvanadium
Copy link
Contributor

okay, heres what
in the regtest case, calling get_transaction_fee :

[zingolib/src/wallet/data.rs:948] self.is_outgoing_transaction() = false
[zingolib/src/wallet/data.rs:949] self.total_value_spent() = 0
[zingolib/src/wallet/data.rs:950] self.value_outgoing() = 0
[zingolib/src/wallet/data.rs:950] self.total_change_returned() = 0
[zingolib/src/wallet/data.rs:948] self.is_outgoing_transaction() = true
[zingolib/src/wallet/data.rs:949] self.total_value_spent() = 625000000
[zingolib/src/wallet/data.rs:950] self.value_outgoing() = 1000
[zingolib/src/wallet/data.rs:950] self.total_change_returned() = 624989000
[zingolib/src/wallet/data.rs:948] self.is_outgoing_transaction() = true
[zingolib/src/wallet/data.rs:949] self.total_value_spent() = 625000000
[zingolib/src/wallet/data.rs:950] self.value_outgoing() = 1000
[zingolib/src/wallet/data.rs:950] self.total_change_returned() = 624989000
[zingolib/src/wallet/data.rs:948] self.is_outgoing_transaction() = false
[zingolib/src/wallet/data.rs:949] self.total_value_spent() = 0
[zingolib/src/wallet/data.rs:950] self.value_outgoing() = 0
[zingolib/src/wallet/data.rs:950] self.total_change_returned() = 0
[zingolib/src/wallet/data.rs:948] self.is_outgoing_transaction() = false
[zingolib/src/wallet/data.rs:949] self.total_value_spent() = 0
[zingolib/src/wallet/data.rs:950] self.value_outgoing() = 0
[zingolib/src/wallet/data.rs:950] self.total_change_returned() = 0
[zingolib/src/wallet/data.rs:948] self.is_outgoing_transaction() = false
[zingolib/src/wallet/data.rs:949] self.total_value_spent() = 0
[zingolib/src/wallet/data.rs:950] self.value_outgoing() = 0
[zingolib/src/wallet/data.rs:950] self.total_change_returned() = 0

@AloeareV
Copy link
Contributor Author

okay, heres what in the regtest case, calling get_transaction_fee :

[zingolib/src/wallet/data.rs:948] self.is_outgoing_transaction() = false
[zingolib/src/wallet/data.rs:949] self.total_value_spent() = 0
[zingolib/src/wallet/data.rs:950] self.value_outgoing() = 0
[zingolib/src/wallet/data.rs:950] self.total_change_returned() = 0
[zingolib/src/wallet/data.rs:948] self.is_outgoing_transaction() = true
[zingolib/src/wallet/data.rs:949] self.total_value_spent() = 625000000
[zingolib/src/wallet/data.rs:950] self.value_outgoing() = 1000
[zingolib/src/wallet/data.rs:950] self.total_change_returned() = 624989000
[zingolib/src/wallet/data.rs:948] self.is_outgoing_transaction() = true
[zingolib/src/wallet/data.rs:949] self.total_value_spent() = 625000000
[zingolib/src/wallet/data.rs:950] self.value_outgoing() = 1000
[zingolib/src/wallet/data.rs:950] self.total_change_returned() = 624989000
[zingolib/src/wallet/data.rs:948] self.is_outgoing_transaction() = false
[zingolib/src/wallet/data.rs:949] self.total_value_spent() = 0
[zingolib/src/wallet/data.rs:950] self.value_outgoing() = 0
[zingolib/src/wallet/data.rs:950] self.total_change_returned() = 0
[zingolib/src/wallet/data.rs:948] self.is_outgoing_transaction() = false
[zingolib/src/wallet/data.rs:949] self.total_value_spent() = 0
[zingolib/src/wallet/data.rs:950] self.value_outgoing() = 0
[zingolib/src/wallet/data.rs:950] self.total_change_returned() = 0
[zingolib/src/wallet/data.rs:948] self.is_outgoing_transaction() = false
[zingolib/src/wallet/data.rs:949] self.total_value_spent() = 0
[zingolib/src/wallet/data.rs:950] self.value_outgoing() = 0
[zingolib/src/wallet/data.rs:950] self.total_change_returned() = 0

value_outgoing doesn't include the transaction fee, the fee is what's left when you subtract the value outgoing + change returned from the value spent. The behavior is correct, it's the name that's misleading. Any idea on what to call this function so that it's clear it doesn't include the fee?

@fluidvanadium
Copy link
Contributor

fluidvanadium commented Oct 10, 2023

non-outgoing transactions return all zeros... but we know this has not been the case in real wallets. leaving some open questions

what conditions cause TransactionMetadata.total_value_spent() to change?

add_spent_nullifier(

what conditions cause TransactionMetadata.value_outgoing() to change?
outgoing_tx_data is written i think also only one place
wallet/transactions.rs 870

add_outgoing_metadata(

what conditions cause TransactionMetadata.total_change_returned() to change?

TransactionMetadata.is_change()

i have not figured out where the latter is written

@AloeareV
Copy link
Contributor Author

what conditions cause TransactionMetadata.total_change_returned() to change?

TransactionMetadata.is_change()

i have not figured out where the latter is written

The last one is written by transactions.rs mark_notes_as_change_for_pool

@fluidvanadium
Copy link
Contributor

why does outgoing require parameters?

@AloeareV
Copy link
Contributor Author

why does outgoing require parameters?

The parameters specify the human-readable part of the address encoding, which we need to check if the address string we sent to is derivable from our keys

@AloeareV AloeareV marked this pull request as draft October 11, 2023 20:02
@zancas zancas closed this Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants