fix(zcoin): correctly track unconfirmed z-coin notes#2331
Merged
Conversation
|
tried to swap ARRR with this, but still getting same error |
borngraced
changed the title
Author
|
borngraced
changed the title
Author
|
@cipig can you please retry again? I've made some changes to this PR 🙏🏾 |
borngraced
force-pushed
the
fix-arrr-note-saving
branch
from
f149bb3 to
e735df9
Compare
borngraced
changed the title
|
started a swap with https://sdk.devbuilds.komodo.earth/fix-arrr-note-saving/mm2_e735df9-linux-x86-64.zip
|
|
|
Author
thanks, does this fix the original issue? |
yes, the last commits fixed the issue #2331 (comment) |
|
ARRR change from swaps is not added back to balance |
Author
Let's try the latest build again 9a76c1b Please provide the log file as well. thanks |
onur-ozkan
reviewed
Feb 3, 2025
mm2src/coins/lp_coins.rs
Outdated
Comment on lines
+2455
to
+2457
| pub fn new_with_unspendable(spendable: BigDecimal, unspendable: BigDecimal) -> CoinBalance { | ||
| CoinBalance { spendable, unspendable } | ||
| } |
There was a problem hiding this comment.
Just my opinion, not a game-changer blocker:
It doesn't feel right to have these functions as CoinBalance fields are already pubed and it doesn't have too many fields.
The use of
let _ = CoinBalance {
spendable,
unspendable
};seems more clean than
CoinBalance::new_with_unspendable(x, y)because we can't know what is the implementation detail behind new_with_unspendable.
dimxy
previously approved these changes
May 31, 2025
Collaborator
dimxy
left a comment
dimxy
left a comment
There was a problem hiding this comment.
Approving.
As a TODO list for this code:
- I think wait_for_spendable_balance_impl loop is better to be integrated with the main light_wallet_db_sync_loop (like all other chain watchers)
- ideally the locked_notes db should be executed within one database transaction together with librustzcash wallet db updates.
- Good to store unconfirmed change output in a db like the locked_notes db, to show accurate unspendable balance
The above 2 issues could be resolved when we migrate to a newer librustzcash which supports tracking spent notes and unconfirmed change.
|
can i please have a new build of this? |
Collaborator
Will check it and merge it asap, sorry that this slipped through the cracks |
Collaborator
I think I should include these as todos in the code instead of opening issues for them. Will do while reviewing. |
…firmed change note tracking
- Restores previous dependency tree
…exedDB schema - Switch LockedNoteTable::on_upgrade_needed to `while old_version < new_version` pattern. - Enables future schema migrations and aligns with IndexedDB best practices.
…sError, remove unused NeededPrevTxConfirmed variant
…nding spent and change notes
Previously, the "unspendable" shielded balance for ZCoin (Sapling) was calculated as the sum of all locked notes' values. This was inaccurate: locked notes represent inputs of pending (unconfirmed) spending transactions, which, once confirmed, will be removed from the balance and replaced by their change outputs. The user should not see the value of spent notes as unspendable, but rather the value of change outputs from pending (unconfirmed) transactions.
This commit refactors the locking database and balance calculation as follows:
- The locked notes tracking now distinguishes between two types:
- `Spent`: notes being spent in a pending tx (should not be shown as available, nor as unspendable)
- `Change`: pending change outputs from unconfirmed txs (should be shown as unspendable until confirmed)
- The balance calculation now:
- Excludes all `Spent` notes from the spendable balance.
- Sums all `Change` notes' values as the true unspendable shielded amount.
- Only change outputs from pending txs are shown as unspendable, providing a more accurate user balance.
This ensures that shielded balances display correctly:
- Spendable: Only notes not being spent by any pending tx.
- Unspendable: Only unconfirmed change outputs.
- Spent notes' value is not shown at all after a send, matching expected wallet behavior.
Collaborator
|
@cipig please test latest commit and that it now produces the correct unspendable balance while keeping the behaviour as before. |
…move redundant `change_value` field
tested and works fine |
shamardy
approved these changes
Jun 13, 2025
shamardy
changed the title
dimxy
pushed a commit
to dimxy/komodo-defi-framework
that referenced
this pull request
Jun 27, 2025
* dev: (30 commits) chore(core): replace hash_raw_entry with stable entry() API (GLEECBTC#2473) chore(core): adapt `MmError` and usages for compatibility with new rustc versions (GLEECBTC#2443) feat(wallet): add `delete_wallet` RPC (GLEECBTC#2497) chore(release): add changelog entries for v2.5.0-beta (GLEECBTC#2494) chore(release): bump kdf version to 2.5.0-beta (GLEECBTC#2492) feat(tests): zcoin unit test to validate dex fee (GLEECBTC#2460) fix(zcoin): correctly track unconfirmed z-coin notes (GLEECBTC#2331) improvement(orders): remove BTC specific volume from min_trading_vol logic (GLEECBTC#2483) feat(ibc-routing-part-2): supporting entire Cosmos network for swaps (GLEECBTC#2476) fix(startup): don't initialize WalletConnect during startup (GLEECBTC#2485) fix(dns): better ip resolution (GLEECBTC#2487) improvement(event-streaming): strong type streamer IDs (GLEECBTC#2441) bump timed-map to `1.4.1` (GLEECBTC#2481) improvement(RPC): unified interface for legacy and current RPC interfaces (GLEECBTC#2450) improvement(tendermint): `tendermint_tx_internal_id` helper (GLEECBTC#2438) feat(walletconnect): add WalletConnect v2 support for EVM and Cosmos (GLEECBTC#2223) feat(ibc-routing-part-1): supporting entire Cosmos network for swaps (GLEECBTC#2459) fix(test): fix HD Wallet message signing tests (GLEECBTC#2474) improvement(builds): enable static CRT linking for MSVC builds (GLEECBTC#2464) feat(wallet): implement HD multi-address support for message signing (GLEECBTC#2432) ... # Conflicts: # mm2src/coins/qrc20.rs # mm2src/mm2_main/src/lp_swap/maker_swap.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #2273