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

ARC-61 - BIP44 Algorand wallet recovery #294

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ehanoc
Copy link

@ehanoc ehanoc commented Apr 15, 2024

Given Algorand's specific features, such as, reyking or closing out addresses (They won't be available in the latest state of the ledger); BIP44 recovery algorithm needs to be adapted if we are to adopt HDWallets into the ecosystem

@ehanoc ehanoc added documentation Improvements or additions to documentation enhancement New feature or request help wanted Extra attention is needed labels Apr 15, 2024
@ehanoc ehanoc self-assigned this Apr 15, 2024
@ehanoc ehanoc changed the title BIP44 Algorand wallet recovery ARC-??? - BIP44 Algorand wallet recovery Apr 15, 2024
ehanoc added 2 commits April 15, 2024 12:59
Signed-off-by: ehanoc <[email protected]>
@HashMapsData2Value
Copy link

We can add as a recommendation that wallets should by-default only allow users to add addresses with a button to increment the key-index or account-number. They should not, by-default, allow users to create an address with some arbitrary derivation path.

This can be left as an advanced feature, where a user can specify a derivation path themselves, e.g. account 34523, index 45098, if they so choose it. The wallet app can have a "create address at custom path" and a "scan address at custom path" functionality. But with the understanding that the user will need to keep track of these themselves and it is not up to the wallet to exhaustively scan through the possibilities.

@ehanoc
Copy link
Author

ehanoc commented Apr 15, 2024

ath themselves, e.g. account 34523, index 45098, if they so choose it. The wallet app can have a "create address at custom path" and a "scan address at custom path" functionality. But with the understanding that the user will need to keep track of these themselves and it is not up to the wallet to exhaustively scan through the possibilities.

Great point. Will add that

@cusma
Copy link
Contributor

cusma commented Apr 15, 2024

Could an empty address, involved in some on-chain activity through fee pooling, be a problem for the activity scanning? E.g. this account is not in the Ledger (never funded with account minimum balance) but has "some" activity in the chain history.

@ori-shem-tov
Copy link
Collaborator

What is the motivation of discovering a closed address? In order for an address to be closed it has to opt out of any asset and smart contract and have algo balance of zero. I would assume closing out an address is the equivalent of deleting it from the account

@yigitguler
Copy link
Contributor

Thank you, Bruno; I am sure this document will reduce the questions and possible wrong implementations in the future.

I believe, we should keep the implementation simple, especially at the beginning phase. Releasing this feature earlier with a small set of functionality is more important than anything, considering the gates it will open.

I think this part is clear; however, it is not the responsibility of the recovery process. At least in our implementation. Because this information can change anytime, without the control of the wallet application:

Algorand supports re-keying of accounts, allowing users to change the signing key associated with their accounts. The BIP44 recovery algorithm should be extended to handle re-keyed accounts, ensuring that the recovered accounts reflect the latest signing key.

@yigitguler
Copy link
Contributor

What is the motivation of discovering a closed address? In order for an address to be closed it has to opt out of any asset and smart contract and have algo balance of zero. I would assume closing out an address is the equivalent of deleting it from the account

I think the idea is to protect users against this scenario:

  • User creates 30 addresses.
  • User closes the first 20
  • User should still be able to access to their 10 active addresses under that account.

@HashMapsData2Value
Copy link

HashMapsData2Value commented Apr 15, 2024

Could an empty address, involved in some on-chain activity through fee pooling, be a problem for the activity scanning? E.g. this account is not in the Ledger (never funded with account minimum balance) but has "some" activity in the chain history.

@cusma How would the address be involved with on-chain activity in this way? If you are referring to a for example privacy scenario where a new address is being funded in an anonymous manner, it should at the end of the group transaction (involving fee pooling) be funded with the amount. Or are you thinking of a different scenario?

ARCs/arc-0300.md Outdated
1. Starting with `account` index 0
2. Scan until no **activity** is found for `gap_limit` consecutive addresses.
- If no **activity** is found on the first scan of the account, stop scanning.
- If addresses have been found to been re-keyed, the wallet should maintain a map of the found address and the corresponding new address / public which is not expected to sign transactions.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should clarify addresses that have been rekeyed to an account address vs account addresses that have been rekeyed. There are three main scenarios.

  1. An external address is rekeyed to an account address within the search. For example, account[19] address has no activity, but there IS an Algorand address rekeyed to account[19]. Do we expect the wallet to find this? If so, should that count as activity for account[19]?

  2. An account address is rekeyed to another account address. In this scenario the wallet should just keep a mapping internally and note the auth addr to the user.

  3. An account address is rekeyed to an external address. In this case the wallet should inform the user that the address is rekeyed, but still count it as activity.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm with Joe here, this second bullet point needs some clarification.

Small nitpick: the following bullet point talks only about the first scan, not the 2nd, 3rd, etc.

If no **activity** is found on the first scan of the account, stop scanning.

Copy link
Author

Choose a reason for hiding this comment

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

Great points @joe-p , will add these cases.

So for 1) It would consider that an address has activity

  1. 👍

  2. 👍

Copy link
Author

Choose a reason for hiding this comment

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

I'm with Joe here, this second bullet point needs some clarification.

Small nitpick: the following bullet point talks only about the first scan, not the 2nd, 3rd, etc.

If no **activity** is found on the first scan of the account, stop scanning.

I don't follow @k13n , there's only 1 overall scan. You "scan" until the first 20 addresses (gap_limit) of an account (bip44 account, no Algorand "account") have no activity. Otherwise you keep moving to the next account

@SudoWeezy SudoWeezy changed the title ARC-??? - BIP44 Algorand wallet recovery ARC-61 - BIP44 Algorand wallet recovery Apr 16, 2024
@cusma
Copy link
Contributor

cusma commented Apr 16, 2024

@cusma How would the address be involved with on-chain activity in this way?

I was thinking about the possibility of an "empty" address to be sender of a transaction by using fee pooling.

@github-actions github-actions bot added the t-arc label Apr 16, 2024
@k13n
Copy link
Contributor

k13n commented Apr 19, 2024

It's good that the ARC defines what counts as activity, it'd also be good to decide how one actually determines if an account has activity. Looking up the account in algod is not sufficient, so we likely need to query the indexer, but what are we looking for exactly?

@pbennett
Copy link
Contributor

Checking algod would tell you if the account exists in any way, 'right now' If the account had been closed though then a historic search would indeed be required (assuming you can get the indexer incantations right to give you only the single most recent transaction).

@ehanoc
Copy link
Author

ehanoc commented Apr 22, 2024

'd also be good to decide how one actually determines if an account has activity. Looking up the account in algod is not sufficient, so we likely need to query the indexer, but what are we looking for exactly?

@k13n @pbennett I would suggest the "how" to be out of the ARC for now. Simply because we don't know what the future will look like (i.e more support from algod and not requiring an indexer) and the standard can remain the same.

thoughts?

@joe-p
Copy link
Contributor

joe-p commented Apr 22, 2024

I was thinking about the possibility of an "empty" address to be sender of a transaction by using fee pooling.

I think the activity criteria should be if the address has been used in any transaction field before (not counting foreign address array). With this definition, even 0 ALGO accounts that sent a transaction would be considered to have activity.

@k13n @pbennett I would suggest the "how" to be out of the ARC for now. Simply because we don't know what the future will look like (i.e more support from algod and not requiring an indexer) and the standard can remain the same.

I think it's worth having a recommendation in the ARC mainly because nothing right now supports the kind of queries we need to do. I think at the very least we will need to create a new indexer endpoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new documentation Improvements or additions to documentation enhancement New feature or request help wanted Extra attention is needed s-draft t-arc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants