feat(btc-provider): accept P2TR accounts and add testnet4#8548
feat(btc-provider): accept P2TR accounts and add testnet4#8548marcopeereboom wants to merge 2 commits into
Conversation
|
FWIW, I tested all of this code using metamask flask and I was able to send and move ordinals. |
c03182b to
0fbd4e0
Compare
|
Addressed the Copilot/Bugbot feedback — it was right. Root cause: Fix: Override Also rebased onto upstream/main to clean out the merge commits. Tests: 3 new (P2TR type preservation, testnet4 scope preservation, P2WPKH/Mainnet identity), full package suite passes (13 suites, 322 tests). |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 0fbd4e0. Configure here.
| scope: account.scopes[0], | ||
| }), | ||
| this.config.createAccounts.timeoutMs, | ||
| ); |
There was a problem hiding this comment.
Resync override doesn't track re-created account IDs
Medium Severity
The resyncAccounts override calls keyring.createAccount(...) directly and discards the returned KeyringAccount. The base class's resync path goes through this.createAccounts() → createBip44Accounts(), which calls this.accounts.add(snapAccount.id) to register the newly created account in the provider's internal tracking set. Since the override skips this, re-created accounts won't appear in getAccounts() or be found by getAccount() after resync completes.
Reviewed by Cursor Bugbot for commit 0fbd4e0. Configure here.
|
The override copy-pastes ~60 lines of the snap-has-more deletion path and the MetaMask-has-more re-creation path from the base class, changing only the re-creation call to use If This also sets a precedent: every new provider that supports multiple address types or network scopes will need its own full override with the same copy-paste pattern (Solana, future account types). Here is an idea on how to fix it; make the base class's re-creation step a virtual method that subclasses can override surgically: // In SnapAccountProvider:
protected recreateAccount(
keyring: RestrictedSnapKeyring,
account: Bip44Account<InternalAccount>,
entropySource: EntropySourceId,
groupIndex: number,
): Promise<void> {
return this.createAccounts({
type: AccountCreationType.Bip44DeriveIndex,
entropySource,
groupIndex,
});
}Then // In BtcAccountProvider:
protected override async recreateAccount(
keyring: RestrictedSnapKeyring,
account: Bip44Account<InternalAccount>,
entropySource: EntropySourceId,
groupIndex: number,
): Promise<void> {
const scope = account.scopes?.[0];
if (!scope) {
throw new Error(
`Account ${account.id} has no scopes, cannot determine target network`,
);
}
await withTimeout(
() =>
keyring.createAccount({
entropySource,
index: groupIndex,
addressType: account.type,
scope,
}),
this.config.createAccounts.timeoutMs,
);
}This keeps all sync logic (deletion, size comparison, error reporting, the |
0fbd4e0 to
c29622e
Compare
Widen BtcAccountProvider to recognize P2TR (taproot) accounts as compatible Bitcoin accounts alongside P2WPKH. Add BtcScope.Testnet4 to the provider capabilities and account discovery scopes. Default account creation remains P2WPKH — no behavioral change for existing callers. P2TR accounts created through discovery or future UI options are now correctly tracked by the provider. Depends on MetaMask/snap-bitcoin-wallet P2TR support for the snap to serve P2TR accounts via keyring_createAccount.
Override resyncAccounts to use the original account type and scope when re-creating missing Snap accounts instead of hardcoded P2WPKH and Mainnet defaults. Guard against accounts with empty scopes to prevent undefined scope in re-creation. Propagate re-creation failure after keyring removal to surface inconsistent state. Align discovery scopes with advertised capabilities by including BtcScope.Testnet. Remove redundant Object.values check in isAccountCompatible.
c29622e to
2adb7c4
Compare


Explanation
BtcAccountProvidercurrently restricts Bitcoin account compatibilityto P2WPKH only and limits capabilities and discovery to mainnet and
testnet3. This blocks taproot (P2TR) accounts from being tracked by
the provider and prevents testnet4 usage.
This PR makes three changes to
BtcAccountProvider:isAccountCompatible: accept P2TR accounts alongside P2WPKH so thattaproot accounts created through discovery or future UI options are
correctly managed by the provider
capabilities.scopes: addBtcScope.Testnet4so the provideradvertises testnet4 support
discoverAccounts: includeBtcScope.Testnet4in discovery scopesso existing testnet4 accounts are found during wallet setup
Default account creation via
createAccountV1remains P2WPKH — nobehavioral change for existing callers. The snap already has full P2TR
and testnet4 support; this change enables the provider to work with
accounts the snap can now create.
References
Checklist
Note
Medium Risk
Medium risk because it changes Bitcoin account compatibility, advertised discovery scopes, and overrides
resyncAccountsto delete/re-create Snap accounts—mistakes could lead to missing or duplicated accounts if scope/type inference is wrong.Overview
Bitcoin provider enhancements.
BtcAccountProvidernow treatsP2TRaccounts as compatible, advertisesBtcScope.Testnet4, and runs discovery acrossMainnet,Testnet, andTestnet4to match its capabilities.Resync behavior change. Overrides
resyncAccountsto re-create missing Snap accounts using the originalaccount.typeand firstaccount.scopesentry (instead of defaulting toP2WPKH/Mainnet), adds guards for empty scopes, and reports/propagates re-creation failures after keyring removal to avoid silent inconsistent state.Tests were updated/expanded to cover P2TR compatibility, multi-scope discovery, and the new resync scenarios.
Reviewed by Cursor Bugbot for commit 2adb7c4. Bugbot is set up for automated code reviews on this repo. Configure here.