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

wallet: allow updating of claimed names #438

Merged
merged 3 commits into from
May 8, 2020

Conversation

tynes
Copy link
Contributor

@tynes tynes commented Apr 27, 2020

There is currently a bug in the wallet where it assumes that REGISTER outputs only come from REVEAL outputs. This prevents any name claim from ever updating their name using the wallet API. It is still possible to manually create the REGISTER transaction and broadcast it directly to the blockchain without using the wallet API. This is risky because a user could forget to create the transaction with the additional output containing the value "locked in the claim" to themselves.

hsd/lib/wallet/txdb.js

Lines 1703 to 1710 in 4178fd3

case types.REGISTER: {
assert(i < tx.inputs.length);
const nameHash = covenant.getHash(0);
const prevout = tx.inputs[i].prevout;
const brv = await this.getReveal(nameHash, prevout);
assert(brv);

REGISTER outputs can also come from CLAIM outputs when the name is reserved.

hsd/lib/covenants/rules.js

Lines 1218 to 1227 in 4178fd3

case types.CLAIM:
case types.REVEAL: {
// Must be be linked.
if (!output)
return -1;
// Reveal has to go to a REGISTER, or
// a REDEEM (in the case of the loser).
switch (covenant.type) {
case types.REGISTER: {

These changes do not use rules.isReserved and instead directly use reserved.has to handle the case when the name is claimed before the claim period ends and then updated for the first time after the claim period ends.

Tests are included that test updating fake claims using the wallet (not the memwallet).

@tynes tynes requested review from boymanjor and pinheadmz April 27, 2020 22:27
lib/wallet/txdb.js Outdated Show resolved Hide resolved
lib/wallet/txdb.js Outdated Show resolved Hide resolved
lib/wallet/txdb.js Outdated Show resolved Hide resolved
lib/wallet/txdb.js Outdated Show resolved Hide resolved
test/wallet-claim-test.js Outdated Show resolved Hide resolved
@pinheadmz
Copy link
Member

I sort of think the test should be written without RPC. It takes the HTTP stuff and API stuff out of the equation. Maybe take a look at the tests in other txdb-focused patches: #262 and #387. If you focus only on the single module you are updating, you may find you can take out a lot of unnecessary stuff like the blockchain! And you may not have to even mine blocks or advance the namestate through its life cycle.

lib/wallet/txdb.js Outdated Show resolved Hide resolved
@tynes
Copy link
Contributor Author

tynes commented Apr 28, 2020

I migrated the tests to wallet-test.js and am no longer using the RPC interface. Still need to add tests that the balances are updating correctly and will remove wallet-claim-test.js.

test/wallet-test.js Outdated Show resolved Hide resolved
test/wallet-test.js Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Apr 29, 2020

Codecov Report

Merging #438 into master will increase coverage by 0.39%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #438      +/-   ##
==========================================
+ Coverage   61.99%   62.38%   +0.39%     
==========================================
  Files         129      129              
  Lines       34843    34843              
  Branches     5920     5922       +2     
==========================================
+ Hits        21600    21736     +136     
+ Misses      13243    13107     -136     
Impacted Files Coverage Δ
lib/wallet/txdb.js 85.04% <100.00%> (+0.50%) ⬆️
lib/covenants/rules.js 73.04% <0.00%> (-0.15%) ⬇️
lib/script/script.js 59.32% <0.00%> (-0.09%) ⬇️
lib/wallet/walletdb.js 78.90% <0.00%> (+0.54%) ⬆️
lib/covenants/namestate.js 88.97% <0.00%> (+1.80%) ⬆️
lib/protocol/consensus.js 89.79% <0.00%> (+2.04%) ⬆️
lib/wallet/wallet.js 73.96% <0.00%> (+4.14%) ⬆️
lib/wallet/nullclient.js 88.37% <0.00%> (+4.65%) ⬆️
lib/blockchain/chainentry.js 79.62% <0.00%> (+20.37%) ⬆️

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 4178fd3...c533bde. Read the comment docs.

@tynes tynes force-pushed the claim-update-fix branch 2 times, most recently from d3f1e8f to 5af133a Compare April 29, 2020 20:17
test/wallet-test.js Outdated 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 Outdated Show resolved Hide resolved
test/wallet-test.js Outdated Show resolved Hide resolved
test/wallet-test.js Outdated Show resolved Hide resolved
The wallet assumed that `REGISTER` outputs could
only come from `REVEAL` outputs, but they can also
come from `CLAIM` outputs. That prevented users
that have claimed their name to ever update it
using the `sendupdate` API. These changes allow
`CLAIM` outputs to be spent using the `sendupdate`
API.
@tynes tynes force-pushed the claim-update-fix branch from 5af133a to 71a6e0f Compare April 29, 2020 20:33
test/wallet-test.js Outdated Show resolved Hide resolved
test/wallet-test.js Outdated Show resolved Hide resolved
@tynes
Copy link
Contributor Author

tynes commented May 1, 2020

We could test this using txdb.abandon().

Added test coverage of this codepath.

@tynes
Copy link
Contributor Author

tynes commented May 1, 2020

Added pre and post assertions for wallet balance after indexing blocks/coins. This should be ready for another round of review @pinheadmz @boymanjor

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

ok this is so close to landing! just down to the nittiest of nits.

@pinheadmz pinheadmz added this to the 2.1.6 milestone May 6, 2020
@tynes tynes force-pushed the claim-update-fix branch from 2cab1c9 to c533bde Compare May 7, 2020 18:40
@tynes
Copy link
Contributor Author

tynes commented May 7, 2020

@pinheadmz @boymanjor Nits are resolved

Copy link
Member

@pinheadmz pinheadmz left a comment

Choose a reason for hiding this comment

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

ACK c533bde

Show Signature
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK c533bdeb9212835e06391163e1cff0f21ba23c04
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAl61UWEACgkQ5+KYS2KJ
yTpUKBAArROU9qIyFdzNEPMdpuljDx1bKCCJafg6+KmAkfuQhMvK/hibrIOoWPSU
DC7JW3tpyxdFczXfzJHRiTuS7l/lu+amthUksCoHyblFYxh/NWRwhPWXBiHVbiKY
lkWPXXBR6fwHSNJDpRuQks5Z6smW8NMm+rrFxezif+z8jVWjd4BnH1leT6pEratA
gVEbrM50t7wX+Jx+MdiLaVI0K2FGLmlmERDBRXJxUjJxEquZ3LDYdiFNBDlFEBOC
GrcGlkGhoTC1aU1hkWm510vev52qVJY2neMVnMwg4JL6v5hj/FlBtZ9ureJ4mIVw
+r/z6XcQ5IeFnzwllxezMgfR4qheL+wOEgLB9nKbn+KKw5QD+APw+kr4sa7s47dM
QEAmjzWkAcyddMnzGzxHppHgbcoxDZUoVSKdVk8DnZn5p6v+wuCJGGX1Lqmz7bXo
pUL7lEW/8xcGWoGnxaBWBwRO0D2hQogjUepdHbd5GqxG6VnPcrDDSP+0G3FtbZfb
vF5mwz11KtwMVIP82ooiZrujO0t+rsdqkdWLOAsckf0PpsCl0a/3iDoZJjIAbo6r
8oJHPkurUrQ9DDJmhtopcHR1Nbrjns6vA85lvwWmI8UM6syDCBf+kkRyEBLTFshx
Uj1q+PgxXSfU2TRROLTaaFQNbK1OX1JSUR2sBGBUt6nTTUwl6Ew=
=IMOH
-----END PGP SIGNATURE-----

pinheadmz's public key is on keybase

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