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

Adjust txdb locked coins balance for FINALIZE covenants #464

Merged
merged 6 commits into from
Sep 3, 2020

Conversation

pinheadmz
Copy link
Member

@pinheadmz pinheadmz commented Jun 1, 2020

I think we may have overlooked the impact of FINALIZE covenants on locked coins balances in the wallet. They are unusual because it is the only case where locked coins leave the wallet. Meaning someone else's locked coin balance goes up. The current txdb functions lockBalances() and unlockBalances() process covenants received into the wallet, however in this case we must also catch FINALIZE as it is being sent, since our own locked coin balance goes down (along with the total balance - name and coins get transferred)

TODO:

  • Evaluate the impact of this change on exiting wallets in use. Do we need another wallet DB migration?
  • Will some users have to completely restore wallet from seed and rescan like the claim-to-register issue?
  • Let's make sure REVOKE and TRANSFER aren't lingering issues as well

According to my covenant statistics script, at this time there are 14 FINALIZE covenants confirmed on the blockchain, for a total value of 3,700.500000 HNS. Both the sender and receiver of these names will have inaccurate locked coin balances, that may end up throwing an assertion error depending on future wallet activity.

Similar PRs recently merged affecting txdb locked coins computation:

#438
#387
#262

@pinheadmz pinheadmz requested review from tynes, tuxcanfly and chjj June 1, 2020 21:28
@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2020

Codecov Report

Merging #464 into master will increase coverage by 0.06%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #464      +/-   ##
==========================================
+ Coverage   62.53%   62.59%   +0.06%     
==========================================
  Files         129      129              
  Lines       34864    34886      +22     
  Branches     5925     5935      +10     
==========================================
+ Hits        21803    21838      +35     
+ Misses      13061    13048      -13     
Impacted Files Coverage Δ
lib/wallet/txdb.js 86.06% <90.90%> (+1.01%) ⬆️
lib/covenants/rules.js 73.04% <0.00%> (-0.15%) ⬇️
lib/covenants/namedelta.js 86.56% <0.00%> (+0.49%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22be7fa...d6282b8. Read the comment docs.

test/wallet-test.js Show resolved Hide resolved
test/wallet-test.js Outdated Show resolved Hide resolved
test/wallet-test.js Outdated Show resolved Hide resolved
test/wallet-test.js Show resolved Hide resolved
lib/wallet/txdb.js Outdated Show resolved Hide resolved
@pinheadmz
Copy link
Member Author

@tynes great feedback thank you! All suggestions taken, pushed two more commits.

@pinheadmz pinheadmz mentioned this pull request Jun 3, 2020
@tynes
Copy link
Contributor

tynes commented Aug 10, 2020

Maybe we could schedule a "review session" for this PR because the longer that we wait to get this in, the more difficult the upgrade will be.

@pinheadmz
Copy link
Member Author

Good idea. I also think that like the change migration bug, we should evaluate and merge this fix ASAP to prevent new wallet corruption in the community, but then we need to figure out if another migration is necessary for wallets that have already been corrupted.

@coveralls
Copy link

coveralls commented Sep 3, 2020

Pull Request Test Coverage Report for Build 238000825

  • 18 of 20 (90.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.8%) to 59.23%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/wallet/txdb.js 18 20 90.0%
Totals Coverage Status
Change from base Build 212167982: 0.8%
Covered Lines: 19387
Relevant Lines: 30471

💛 - Coveralls

@pinheadmz
Copy link
Member Author

Ok after sitting on this for a long time, I think we should merge as is. I tried to play with a migration fix this morning but I think the best method will be a Deep Rescan and again we'll just have to explain in the CHANGELOG

@pinheadmz pinheadmz merged commit 7826128 into handshake-org:master Sep 3, 2020
@pinheadmz pinheadmz added this to the v2.3.0 milestone Sep 17, 2020
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.

4 participants