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

Bugfix: Clarification of Amounts in tables (fixes #1861) #2026

Merged
merged 45 commits into from
Jan 20, 2023

Conversation

k9ert
Copy link
Collaborator

@k9ert k9ert commented Dec 20, 2022

This PR addresses criticism on how amounts are shown and fixes some edge-cases around that and does some refactoring around txlists. In detail:

  • Outflowing value is shown as a negative number
  • Outflow includes the fee.
  • If Txs have inputs which doesn't belong to the wallet, wrong numbers were shown. This is fixed now.
  • There is also a clear-cache button in the settings of each wallet.
  • Some refactorings
    image

Before (history):
image
After (history):
image

Before(utxo)
image
After (utxo):
image

@netlify
Copy link

netlify bot commented Dec 20, 2022

Deploy Preview for specter-desktop-docs canceled.

Name Link
🔨 Latest commit e803786
🔍 Latest deploy log https://app.netlify.com/sites/specter-desktop-docs/deploys/63ca4ceca2b8ae00081e4bfb

wallet: Wallet = app.specter.wallet_manager.get_by_alias(wallet_alias)
error = None
wallet.clear_cache()
flash("Cache cleared successfully!")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not so clear to the user which cache this is referring to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The tooltip looks like this:
image

@moneymanolis
Copy link
Collaborator

With the PR checked out, my wallets are not loading completely (works fine on master):

grafik

probably due to this NameError:

grafik

@moneymanolis
Copy link
Collaborator

moneymanolis commented Dec 20, 2022

There is also a clear-cache button in the settings of each wallet.

Is this pushed already, can't see anything in this regard?

We now try to implement bigger chunks of functionality in plugins. Maybe, even core functionality might be implemented in "core plugins" in the future. Internal plugins are placed in `src/specterext`. But, plugins can also live in there own repos, have their own release lifecycle and be used by Specter like any other dependency. For more information about plugins see [Third Party Service Integrations](./extensions.md) where we also discuss the nuances between plugins and extensions.

# The Crypto Engine
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure whether this is the right expression ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure what you're referring to. What's wrong here? I added src/cryptoadvance/specterext but apart from that?!

Copy link
Collaborator

@moneymanolis moneymanolis left a comment

Choose a reason for hiding this comment

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

Export of transaction history (completely empty CSV) and UTXOs (only headers) is not working.

@moneymanolis
Copy link
Collaborator

With the PR checked out, my wallets are not loading completely (works fine on master):

Can't reproduce. Can you check that again?

Works now.

@k9ert
Copy link
Collaborator Author

k9ert commented Jan 13, 2023

as predicted @moritzwietersheim (from the failing cypress-tests) :


[  ERROR] in      controller: Traceback (most recent call last):
  File "/tmp/cirrus-ci-build/.env/lib/python3.10/site-packages/flask/app.py", line 1523, in full_dispatch_request
    rv = self.dispatch_request()
  File "/tmp/cirrus-ci-build/.env/lib/python3.10/site-packages/flask/app.py", line 1509, in dispatch_request
    return self.ensure_sync(self.view_functions[rule.endpoint])(**req.view_args)
  File "/tmp/cirrus-ci-build/.env/lib/python3.10/site-packages/flask_login/utils.py", line 269, in decorated_view
    return func(*args, **kwargs)
  File "/tmp/cirrus-ci-build/src/cryptoadvance/specter/server_endpoints/wallets/wallets_api.py", line 397, in txlist
    txlist = wallet.txlist(
  File "/tmp/cirrus-ci-build/src/cryptoadvance/specter/wallet.py", line 1083, in txlist
    self.fetch_transactions()
  File "/tmp/cirrus-ci-build/src/cryptoadvance/specter/wallet.py", line 395, in fetch_transactions
    unconfirmed_selftransfers = [
  File "/tmp/cirrus-ci-build/src/cryptoadvance/specter/wallet.py", line 398, in <listcomp>
    if self._transactions[txid].category == "selftransfer"
AttributeError: 'LTxItem' object has no attribute 'category'
[WARNING] in          txlist: this can probably be ignored: Failed to rewind the proof
[  ERROR] in      controller: Uncaught exception: 'LTxItem' object has no attribute 'category'

If it's cheap for me to fix that, i'll do it.

@k9ert
Copy link
Collaborator Author

k9ert commented Jan 16, 2023

Ok, Liquid fixes were not that difficult. At least not the use-cases qhich the tests cover. Should be all green now!

@moneymanolis
Copy link
Collaborator

Export of transaction history (completely empty CSV) and UTXOs (only headers) is not working.

Export of transaction history works now but export of UTXOs is still only headers.

@moneymanolis
Copy link
Collaborator

The csv cache seems to be overwritten this is how it looks like, is this all as intended? @k9ert

txid,fee,blockhash,blockheight,time,blocktime,bip125-replaceable,conflicts,vsize,address,category,flow_amount,utxo_amount,ismine
1a5206140a136c6e4d1901923d7fa765918904bb235bdae43de336e92507304a,,132aae8511edbd3f6ac52c6e253e1dade0eebbe1635766cf5a66b955399a7772,735,1620068439,,no,[],,"['bcrt1q6x05wm2r8ey6hztcz2l5quze7svkemjrms98he', 'OP_RETURN 6a24aa21a9edcf8fb925b7f34943bf39c7d5bcd1ba9378fd6bd936f3e0c7dfc68443623c2e3b']",generate,3.12500208,,True
1e488de679b96d87c5c6377fb81884d0b3695f417305de4d68a3a634129cdb1d,,174ecbe9cc62cbc2b51312a6d30b79234106add4a186b511c7e51c49bdc2b610,736,1622573532,,no,[],,"['bcrt1qp3e6thsy9e538llhj4fgze5m5c7usz8qz3tqdc', 'bcrt1q0ktwqaptyg6gveg03lg8q9hs6qxwtlkrwd5ysv']",send,-100.0003425,,True
2cba28d71bb1693e3c29f59d424fa23cf9ad113dfb905c39ba711cffd88a6f5a,,666f308fa50539f8f030bcbd338a1ea20af3f06d39a014fc7b1281ac7b2e5f09,746,1622573915,,no,[],,"['bcrt1qn0maknxzxpyw04xmaelquf2f079wmfpkpw8x4v', 'bcrt1qwkeamkcyttskdm46jdkye72fwm73ycc9lnlg8e']",receive,0.001,,True
fd8f82a067c4bf9115ddba25054ee0e76e08825d1630a8d3ea37e48091565ab0,,5f80aa6b1e346545ba6ab3c68f47d12375ec5ef789a0fe60f87ce3abfa3bf40b,747,1636826971,,no,[],,bcrt1q78lx4k2u92f2w0nyc88qxlcfga55spdgma8868,selftransfer,-1.4100000003125501e-06,,True
6fafeb80c4f8797c86519b9b42f9056f671b2c5f28e62660a1d86ee3cf52ba11,,5f80aa6b1e346545ba6ab3c68f47d12375ec5ef789a0fe60f87ce3abfa3bf40b,747,1636827151,,no,[],,"['bcrt1qtcrknq74ju8fhn48f08g246wcdea3ag8p2q0kc', 'OP_RETURN 6a24aa21a9ed49618264fd14f9d625bc87280494386150154140ace453869f556e285b4df18e']",generate,3.12500141,,True
6b80e12af1acb544c703ad6bcfefdd5eae729127b064d300c9029f0c7e3dfbd1,,7db38b0a4f3fa3048c1b9528acf327e65c8f4338ce045d80a04a0a5102d15d2e,748,1636827151,,no,[],,"['bcrt1qtcrknq74ju8fhn48f08g246wcdea3ag8p2q0kc', 'OP_RETURN 6a24aa21a9ede2f61c3f71d1defd3fa999dfa36953755c690689799962b48bebd836974e8cf9']",generate,3.125,,True
2812cef72a307ab48adf94a238fd5d73210b7240db7c76c5c587a02aca8cfd2a,,517858df7200f7e5c9882fbebcbf2d04cd8c05cf2bc26e9d2b9485d21b9ddce0,749,1636827151,,no,[],,"['bcrt1qtcrknq74ju8fhn48f08g246wcdea3ag8p2q0kc', 'OP_RETURN 6a24aa21a9ede2f61c3f71d1defd3fa999dfa36953755c690689799962b48bebd836974e8cf9']",generate,3.125,,True

@moneymanolis
Copy link
Collaborator

UTXO list export works now!

@k9ert
Copy link
Collaborator Author

k9ert commented Jan 20, 2023

As discussed in the review, the case for 2 outputs in an incoming transaction into the same wallet might cause headache with coin-selection (the first would win, the second is not visible). So i reimplemented the whole method the other way around:
f1fb38e

Now, if there are two utxos with the same txid but two vout, this will properly result in two WalletAwareTxItem both with the same txid but different vout.
CC @moneymanolis

@k9ert k9ert merged commit fa9605d into cryptoadvance:master Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants