Skip to content

Conversation

Quexington
Copy link
Contributor

@Quexington Quexington commented Jun 26, 2025

This PR removes all user endpoints for initiating recovery related actions. It does not, however, remove the concept of recovery entirely as this is part of the puzzle and must be maintained to sync DIDs. It removes the ability to set recovery info on new DIDs, change it on existing ones, or take part in a recovery of a DID.

The motivation behind this PR is that the feature is rarely, if ever, used and creates a lot of wallet surface area even so. The code that supports it is shaky and not well tested, so encouraging further use is likely more harmful than just removing it entirely. Should any user become stranded by this -- which I find quite unlikely -- and actually cares about the DID -- also unlikely -- it is easy enough to downgrade to a previous version to initiate a recovery before returning to a newer version.

(Reviewing the test changes is a little awkward because of how GitHub calculated the diff. I'm not sure how to improve this.)

@Quexington Quexington requested a review from a team as a code owner June 26, 2025 13:26
@Quexington Quexington added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Jun 26, 2025
@Quexington Quexington changed the base branch from main to quex.port_did_rpcs_to_marshal June 26, 2025 13:26
@Quexington Quexington closed this Jun 26, 2025
@Quexington Quexington reopened this Jun 26, 2025
@arvidn
Copy link
Contributor

arvidn commented Jun 26, 2025

what kind of evidence do we have that this is rarely used? Has anyone looked for these kinds of spends on the blockchain?

@Quexington
Copy link
Contributor Author

what kind of evidence do we have that this is rarely used? Has anyone looked for these kinds of spends on the blockchain?

I will cite 3 pieces of evidence:

  1. Testimony by those close to the community. There are never any support requests, and nobody has brought it up or asked for it in newer wallets.
  2. The code quality is part of why I'm deleting it, and it lacks so much test coverage that it almost certainly has bugs. If people were using it, we would have almost certainly received bug reports.
  3. DIDs aren't at all used in the way this feature is set up for. This is not just a recovery method that you set up for yourself, the idea is that it's a "social" recovery in where you identify a set of custom trusted individuals who can recover your identity for you should you lose access to it. This is very important if you have infrastructure tied to this identity, but I've been told that in contemporary use DIDs are basically "glorified folders for NFTs". Losing access to one isn't even really a big deal since you can just make a new "folder" should you need it. It does not prevent you from spending your NFTs.

@Quexington Quexington changed the title [CHIA-3098] Delete DID recovery endpoints [CHIA-3251] Delete DID recovery endpoints Jun 27, 2025
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jul 1, 2025
Base automatically changed from quex.port_did_rpcs_to_marshal to main July 8, 2025 16:58
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Jul 17, 2025
Copy link
Contributor

File Coverage Missing Lines
chia/wallet/did_wallet/did_wallet.py 75.0% lines 226
chia/wallet/wallet_request_types.py 75.0% lines 1008
chia/wallet/wallet_rpc_api.py 80.0% lines 1105
Total Missing Coverage
60 lines 3 lines 95%

Copy link

Pull Request Test Coverage Report for Build 16342035198

Details

  • 57 of 60 (95.0%) changed or added relevant lines in 7 files are covered.
  • 62 unchanged lines in 10 files lost coverage.
  • Overall coverage decreased (-0.06%) to 91.314%

Changes Missing Coverage Covered Lines Changed/Added Lines %
chia/wallet/did_wallet/did_wallet.py 3 4 75.0%
chia/wallet/wallet_request_types.py 3 4 75.0%
chia/wallet/wallet_rpc_api.py 4 5 80.0%
Files with Coverage Reduction New Missed Lines %
chia/full_node/pending_tx_cache.py 1 96.55%
chia/wallet/util/wallet_sync_utils.py 1 86.07%
chia/timelord/timelord_launcher.py 2 90.0%
chia/wallet/wallet_state_manager.py 2 96.12%
chia/wallet/wallet_node.py 3 87.6%
chia/server/node_discovery.py 4 81.75%
chia/wallet/wallet_rpc_api.py 6 91.89%
chia/wallet/did_wallet/did_wallet_puzzles.py 8 76.14%
chia/wallet/did_wallet/did_wallet.py 13 91.65%
chia/_tests/core/util/test_lockfile.py 22 77.31%
Totals Coverage Status
Change from base Build 16332491766: -0.06%
Covered Lines: 101928
Relevant Lines: 111503

💛 - Coveralls

Copy link
Contributor

@geoffwalmsley geoffwalmsley left a comment

Choose a reason for hiding this comment

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

We'll need to make sure that the relevant areas of the documentation are updated to reflect the removed endpoints, and provide instructions that recovery is done by downgrading to a specific version.

@Starttoaster Starttoaster merged commit 191ccde into main Jul 18, 2025
519 of 521 checks passed
@Starttoaster Starttoaster deleted the quex.delete_did_recovery_endpoints branch July 18, 2025 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changed Required label for PR that categorizes merge commit message as "Changed" for changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants