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

CHIP-0002: Add dApp protocol #9

Merged
merged 33 commits into from
Oct 25, 2022
Merged

Conversation

dimitrysuen
Copy link
Contributor

No description provided.

@Hemasecret
Copy link

It's an important protocol, hope go through ASAP.

Copy link
Contributor

@mariano54 mariano54 left a comment

Choose a reason for hiding this comment

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

Thank you very much for getting started on this initiative. I think standardizing such an API will be crucial for accelerating developer adoption in Chia.

We have to be careful, since the spec that we choose might become standard for months or years, and will be hard to update. There are many considerations we must think about, one of which is to support a wide range of dapps. The signature APIs are a good start to be able to support arbitrary dapps. I have added some preliminary comments, but this needs a lot more review.

  1. Can you provide details on what the main goals, principles, and requirements are for this API?
  2. What other web3 APIs is this based off of? In which ways does it deviate from them?

CHIPs/chip-0002.md Show resolved Hide resolved
## Methods

### accounts
To get the user's identity and check the dApp accessible. API returns the standard pay puzzleHash if dApp is approved. Otherwise, it returns the empty list. For the single address wallets, API returns one puzzleHash. For the multiple address wallet, API returns a list of puzzleHash.
Copy link
Contributor

@mariano54 mariano54 Apr 22, 2022

Choose a reason for hiding this comment

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

Three are many things to consider for this API.

  1. Is there a limit to number of puzzle hashes? If using many addresses, it could be hundreds or thousands.
  2. Is there a primary address to identify the user? Maybe the first address? My preference is no, that we should instead hash the root public key and use that as the main identifier for the user. That should probably be a new API method. (keys api and addresses api).
  3. Should we use observer keys and just return the root public key? Then the website can use that as the identifier, and derive all the user's puzzle hashes from that. If so, then what do we do about the non-observer addresses?
  4. Does this only work for standard wallet accounts? also for CATs? Can the user use a custom PH? if we return address yes, but if we are returning only public keys, no. We might want to return a JSON dict for every address, which includes the type of address (standard, CAT, etc) as well as the public key.
  5. There is a potential privacy issue if we are telling this site about all of our addresses. Perhaps we only want to spend from 1 of them, and we don't care about the other ones. Maybe that's fine, and if we require funds, we can skip calling the accounts method, and instead ask for the funds or the balance.

Copy link
Contributor

@mariano54 mariano54 Apr 22, 2022

Choose a reason for hiding this comment

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

  1. When we return addresses, we should only return all the addresses that have been used so far. For example, let's say I used addresses 1-200, and the wallet has addresses up to 400 created but not yet used. When the user clicks "generate new address" we would return #201. When the JS calls accounts (should be renamed to addresses) endpoint, we would return addresses 1-201, which are the addresses that the user has used so far. Perhaps there should also be a method to request a new address, for example if the user is getting paid for something. Address reuse is bad practice and it's not good for privacy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Yes.
    Because of browsers' precious storage and computing resources and the limitations of hardware wallets, we plan to limit the maximum number. The number will rely on the wallet's implementation. 50-100 is a range of the number we recommend. Users use one by default but can add a new puzzle hash.
    We want the dApp to decide for itself to choose one from the address as the user identifier.

  2. We think using the address is simpler and more meaningful. And hashed root public key is not helpful for the dApps. We shouldn't expose the keys API. keys should be managed inside the wallet.

  3. It's similar to 2. IMO, dApps needn’t distinguish between observer and non-observer addresses. If necessary, a new API can be provided separately to expose the root public key.

  4. That makes sense. We updated it in the proposal.
    {puzzleHash: string, publickKey: string, hdPath: string}

  5. & 6. It seems that exposing the user's address is inevitable. We are still thinking about this. Haven't figured it out yet.

Copy link

@stmharry stmharry Apr 26, 2022

Choose a reason for hiding this comment

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

I think there might be three "address modes" that we are mixing up in this discussion.

  1. The use-and-dump address protocol -- as implemented in the official Chia wallet
  2. Fix-sized SPV address set -- which I believe Goby is targeting
  3. Single-addressed SPV -- the implementation is easy but not really secure if we ever encounter a AGG_SIG_UNSAFE condition

For extension/web/app-based wallets I think (2) is likely the way to go -- the clients cannot really afford to keep track of hundreds of addresses, nor can the current full nodes, without decent modification, sustain continued calls from thousands of clients with their respective 100+ addresses. In this case, there are two pathways that makes sense to identify a wallet (or sub-wallets/accounts): (A) in the case of hardened derivation, we can only use leaf public addresses as there's no pubkey derivation, and (B) in the case of unhardened derivation, we able to use the wallet master pubkey.

But even we are able to use the root or wallet master pubkey in (B) does not mean we should. I think most end-user do not wish to auto opt-in exposing their observer keys. So maybe returning SPV addresses would make sense in this context.

Copy link
Contributor

@mariano54 mariano54 Apr 26, 2022

Choose a reason for hiding this comment

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

Thank you for the good points. I think the address modes should be chosen by the specific wallet, and the spec should support all.

  1. With regards to the global identifier, I agree revealing master PK has potential risks so we should not do that. However, this is why i suggested H(master public key). This can be generated and returned by the wallet, and does not leak any info. Alternatively, we could return the 0th standard observer address and use that as the identifier, but that does leak some information, and also depends on the standard puzzle format. We really DO need a global identifier that is consistent across dapps. In ethereum, this is the address itself. This allows websites to store information about that wallet, using a certain primary key, for example. For example: consider a dapp that displays your TXs in html, and allows downloading a CSV file of them. You want to name the file: GLOBAL_ID.csv. if there is no global ID, there is no way to differentiate two users. If the user logs in to a different key, then that should be a different user.
  2. As for the limits in number of addresses, some users already have more than 100 addresses derived from their sk, so they would not be able to use wallets on this standard. Therefore the standard should not set a limit, but each wallet should have it's own limit. Hardware wallets and other wallets don't need to use more than 100 addresses, they can always choose to not regenerate new ones.
  3. It seems like the spec only returns unwrapped addresses (not CAT wrapped addresses for example). I think this is fine. I also think it's fine that the spec does not specify the actual puzzle that is being used, since this can be at the discretion of the wallet. We should add a comment about this in the doc as well.
  4. How about adding an argument which specifies if we need the PK or not? Returning the PK always does leak unnecessary info.
  5. Finally, is there a rationale for merging all master keys into one? In metamask, you log in to each key separately, and it makes sense that each one is viewed separately no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. We've updated and specified it in the proposal.
  2. We prefer to add a method like getPublicKeyByAddress. If we add an argument, the dApp will definitely require PK information by default.
  3. Yes. A different key has a different global identifier.

CHIPs/chip-0002.md Outdated Show resolved Hide resolved
CHIPs/chip-0002.md Show resolved Hide resolved
CHIPs/chip-0002.md Outdated Show resolved Hide resolved
CHIPs/chip-0002.md Outdated Show resolved Hide resolved
CHIPs/chip-0002.md Outdated Show resolved Hide resolved
| coins | The coins will be used in the spend bundle. |
| assetId | The asset id in the CAT1 standard. It will be '' if the user selects native tokens. |
| delta | `coinsAmount - (paymentAmount + fee - delta) = change` |
| tail | The field is the limitation program of `cat.` |
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need tail and tailSolution, doesn't the wallet know what these are?

Copy link
Contributor Author

@dimitrysuen dimitrysuen Apr 25, 2022

Choose a reason for hiding this comment

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

It's used in CAT's puzzle, e.g., mint or melt.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we specify which things are Optional and which aren't?


> Need discussion:
>
> whether support sign `AGG_SIG_UNSAFE`
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like signTx corresponds to AGG_SIG, and signMessage corresponsds to AGG_SIG_UNSAFE right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also we should allow passing in a list here, we don't want to requst 3x approval from the user, if we have to sign with 3 keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also we should allow passing in a list here, we don't want to requst 3x approval from the user, if we have to sign with 3 keys.

Coinspends is a list. We can put all of them in it.

Copy link
Contributor Author

@dimitrysuen dimitrysuen Apr 25, 2022

Choose a reason for hiding this comment

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

It seems like signTx corresponds to AGG_SIG, and signMessage corresponsds to AGG_SIG_UNSAFE right?

signTx can sign all the coinSpends, which consists of AGG_SIGN_ME and AGG_SIGN_UNSAFE. AGG_SIGN_ME is safe, while AGG_SIGN_UNSAFE is unsafe. Can we just disable AGG_SIGN_UNSAFE?

Choose a reason for hiding this comment

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

I hope AGG_SIG_UNSAFE can be ignored for SPV-like wallets. Not only are they not recommended, but also do not have cost advantages. On top of it, it makes SPV wallet devs have nightmares.

Copy link
Contributor

Choose a reason for hiding this comment

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

signMessage is similar to AGG_SIG_UNSAFE right (no coin ID)? so the dapp can always get a signature through that route anyway.

I think AGG_SIG_UNSAFE is an important opcode that allows things like two users spending the same singleton concurrently. For example in the AMM case, they would both sign something like (singleton_id, min_price, max_price, amount ...) etc. But that does need UNSAFE

Copy link
Contributor Author

@dimitrysuen dimitrysuen Apr 27, 2022

Choose a reason for hiding this comment

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

Yes. If it is supported, the dApp can create a specific format (e.g., data + coinId + networkPrefix), which is the signature of a coin.
In this case, the dApp could do evil. We don't have a good way to prevent it.

Copy link
Contributor

Choose a reason for hiding this comment

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

AGG_SIG_UNSAFE is not always an issue, as there are other ways to provide entropy to ensure it's unique per address and can't be reused. It just has to be used properly for this to be the case, which is why it's named "UNSAFE". It's more of a warning than a deprecation.

@mariano54
Copy link
Contributor

mariano54 commented Apr 22, 2022

The problem of having a global identifier for each user is important. I think it should be sha256(root_public_key). This means each user can have multiple keys, and each key will have it's own information. Maybe everything needs to have a `key_id" parameter as well, to support multiple keys.

Multi-account users are pretty common. It would be nice to be able to fetch balances for all accounts at once, but that could be tricky with hardware wallets. I think the best option might be to just be able to just provide the key_id with every request

@mariano54
Copy link
Contributor

The difference between ETH and Chia is that in ETH there are only accounts (a list of addresses). In chia there are two levels: keys and addresses. So the accounts endpoint should be expanded to:

  • keys (no key_id provided)
  • addresses (key_id provided)

@mariano54
Copy link
Contributor

We might want to have a get_peak method which allows you to see how synced the wallet is, for example the wallet might be behind.

@fizpawiz
Copy link

The issue of global identifiers for users is at the heart of the ongoing DID work. We should make sure to consider that work wherever we are thinking about "users" or "accounts".

CHIPs/chip-0002.md Outdated Show resolved Hide resolved
```

### selectAssetCoins
API returns the spendable coin amount for the selected CAT.
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this also return the spendable balance for NFTs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. We're still working on DID and NFT.


> Need discussion:
> how to support BLS multi-sig?
> how to avoid sign tx data?
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe prefix and postfix every message with some fixed value in the wallet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're considering sha256("\x18Chia Signed Message:\n" + len(message) + message).

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to prefix something, we should use the network_id or genesis_challenge to prevent replay attacks between mainnet, testnet, and forks.

Also, aren't there usecases where we need the actual signature of some object without a prefix, or are these all handled by signTx?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We considered adding networkIds but eventually removed them because this function is essentially a proof of identity for the user's public key. The dApp verifies the identity by the public key. We will highlight in the documentation that dApp can prevent replay attacks by adding information such as timestamp and network id into the message. For some dApps, they don't care which chain they are on, they just need to include the timestamp in the message.

Choose a reason for hiding this comment

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

We're considering sha256("\x18Chia Signed Message:\n" + len(message) + message).

What's the rationale for the \x18 byte? If it's an homage to \x18Bitcoin Signed Message:\n, then my understanding is that the 0x18 represents the length of the string "Bitcoin Signed Message:\n" (24 in decimal). So I would expect the Chia prefix to be "\x15Chia Signed Message:\n" to properly encode the length of the "Chia Signed Message:\n" string.

I found the details mentioned at: https://blockforums.org/topic/98-signed-message-verification-library/?do=findComment&comment=1872

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're considering sha256("\x18Chia Signed Message:\n" + len(message) + message).

What's the rationale for the \x18 byte? If it's an homage to \x18Bitcoin Signed Message:\n, then my understanding is that the 0x18 represents the length of the string "Bitcoin Signed Message:\n" (24 in decimal). So I would expect the Chia prefix to be "\x15Chia Signed Message:\n" to properly encode the length of the "Chia Signed Message:\n" string.

I found the details mentioned at: https://blockforums.org/topic/98-signed-message-verification-library/?do=findComment&comment=1872

Hey Jeff.

Yes. You’re right. Thanks for pointing this out. The magic number in BTC/ETH is the length of the network name.

Actually, at the first beginning, we chose 0x18 because of RFC 4880. And the number doesn't affect the security.

After consideration, we decide to change to shatree(("Chia Signed Message" . message)).

>
> should the wallet track this kind of tx?

### getPublicKey
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 not sure whether the public key should be exposed at all.
Can someone think of a use case where this is necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, there's a privacy benefit in only revealing puzzle hashes and never the pk

Copy link
Contributor Author

@dimitrysuen dimitrysuen Apr 26, 2022

Choose a reason for hiding this comment

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

This is related to the sign message method.
For example, if we wanna log in to a website, such as https://chat.blockscan.com/, the user's address (standard puzzle hash or public key) is usually used as the identity; the backend will need to verify that the user owns it. The website will ask the user to sign a specific message with the private key. Then the signature, the message, and the public key are sent to the server, which can be used to verify if the user signs the signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our original idea is to maximize the flexibility of dApps to generate arbitrary user addresses, such as the farmer public key and pool public key, with user authorization. The dApps can use these public keys in their own puzzles.

Copy link
Contributor Author

@dimitrysuen dimitrysuen Apr 26, 2022

Choose a reason for hiding this comment

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

Here are two thoughts.

  1. dApps may need the public key to verify the signed message
  2. We considered using ASSET_PUZZLE_ANNONUCEMENT + user's puzzle_hash to do the authentication, which will cost at least one standard coin. However, if we use AGG_SIGN_ME + pk, we can save the standard coin.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't identities be done with DIDs though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's optional. Under some scenarios, this solution is more suitable.

@dimitrysuen
Copy link
Contributor Author

Thank you very much for getting started on this initiative. I think standardizing such an API will be crucial for accelerating developer adoption in Chia.

We have to be careful, since the spec that we choose might become standard for months or years, and will be hard to update. There are many considerations we must think about, one of which is to support a wide range of dapps. The signature APIs are a good start to be able to support arbitrary dapps. I have added some preliminary comments, but this needs a lot more review.

  1. Can you provide details on what the main goals, principles, and requirements are for this API?
  2. What other web3 APIs is this based off of? In which ways does it deviate from them?

Hi, @mariano54. Thanks for the comments. We'll add these parts in our latest PR.

@dimitrysuen
Copy link
Contributor Author

We might want to have a get_peak method which allows you to see how synced the wallet is, for example the wallet might be behind.

We considered this method before but then removed it. The wallet should be available even if it is behind. The wallet can show this peek, but dApp doesn't need to know this information.

@dimitrysuen
Copy link
Contributor Author

The issue of global identifiers for users is at the heart of the ongoing DID work. We should make sure to consider that work wherever we are thinking about "users" or "accounts".

Hi, @fizpawiz. We recommend that dApps choose a method for global identifiers, such as address, DID or public key, according to their own scenarios.

@dimitrysuen dimitrysuen changed the title CHIP-0002: Add dApp protocol CHIP-TBD: Add dApp protocol Apr 26, 2022
CHIPs/chip-0002.md Outdated Show resolved Hide resolved
| payment.amount | The amount of payment, unit is `mojo` |
| payment.memos | The memos of payment, the value is a list of hex strings. |
| fee | The field is valid when `assetId` is an empty string. |
| coins | The coins will be used in the spend bundle. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these necessary or optional?

|-----------|------------------------------------------------------------------------------|
| to | bech32-encoded address with network prefix |
| amount | the transfer amount, unit is `mojo` |
| assetId | the asset id in the CAT1 standard. It is" when transferring native tokens. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Would a type parameter be useful to know what type of transfer we are making? e.g. CAT1, CAT2, NFT1.

We should be able to add support for new things later on without breaking changes or having a weird legacy field left over from a previous version.

My concern is if CAT2 also has asset ID and we need to know which one to use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll add a method or other parameters if the type parameter can't meet the needs in the future.

CHIPs/chip-0002.md Outdated Show resolved Hide resolved
Return a list of `standard puzzleHash`. The number of addresses returned depends on the wallet implementation. If dApp is not approved, API will return an empty list.

```tsx
addresses(): string;
Copy link
Contributor

@freddiecoleman freddiecoleman Apr 27, 2022

Choose a reason for hiding this comment

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

Suggested change
addresses(): string;
addresses(): Promise<string[]>;

I'm guessing all the other methods should also return promises.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the functions discussed here will be wrapped by window.chia.request(args: RequestArguments): Promise<any>;. So we needn't add the Promise for this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. In that case maybe the Promise<any> could be changed to use a generic to make it more type safe.

CHIPs/chip-0002.md Outdated Show resolved Hide resolved
@danieljperry
Copy link
Contributor

danieljperry commented Apr 28, 2022

Thanks to Dimitry for starting this CHIP, and thanks to everyone for your helpful reviews!
This CHIP is now a DRAFT. I will act as its Editor. My job here is to be a neutral arbiter, and not to interject my opinions. But please do let me know if there's a dispute for me to resolve.

Before this CHIP can be moved to REVIEW, Dimitry will still need to add a few sections from the CHIP template, and there will need to be extensive review by the community. However, the time has come to move most of the discussions away from this PR (existing comments won't be removed and should be thoroughly addressed).

For less formal discussion of this CHIP, please head to the Keybase channel called "chia_network.public.dapp_protocol". Let me know if you need access. And feel free to invite anyone who might be interested in contributing to the discussion.

You should still leave your formal reviews in this PR. This CHIP might be a draft for a while, so feel free to leave multiple reviews if needed.

## Methods

### addresses
Return a list of `standard puzzleHash`. The number of addresses returned depends on the wallet implementation. If dApp is not approved, API will return an empty list.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we define standard puzzle hash? If the wallet wants to use a different type of puzzle than another wallet, that's fine right?

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 just say: XCH address. XCH will be sent directly to this address, so this should not be a wrapper CAT address, for example


### connect

The dApp requests users' permission to connect the wallet. If the user rejects the request, the API will throw `userRejectdRequestError`.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does connect imply? Does this give read access to all the users data? It definitely does not give write access, correct?

Return the current chainID.

| CHAIN NAME | CHAINID |
|------------|-----------|
Copy link
Contributor

Choose a reason for hiding this comment

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

we might want to specify "Chia Mainnet" on the left

|-----------|----------------------------------------------------------------------------------------------------------------------------------------------|
| assetId | The CAT1 asset id. It will be `null` if the user selects native tokens |
| amount | This is optional. All the coins(included locked coin) will be returned if it is missing. Otherwise, API will return the unlocked coins only. |
| excludes | This is optional. This value is the `name` of coins. |
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to include_coin_name?

coinName: string;
puzzle: string;
confirmedBlockIndex: number;
timestamp: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need the timestamp?

selectAssetCoins(params: SelectAssetCoinsParams): CoinRecord[]
```

### getAssetBalance
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just a convenience method, because we can call selectAssetCoins and add up the amount right?

method: string;
params?: object;
}
window.chia.request(args: RequestArguments): Promise<any>;
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 make it clear that any request might take a while (Several minutes) because the user has to approve manually. Is this true?


interface PushTransactionResp {
// mempool status, success/pending/failed
status: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a list of statuses, because it's possible the wallet might be connected to multiple nodes in the future.


### addressesChanged

the bridge emits `addressesChanged` if the addresses change account in the wallet
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this mean? Does it mean a new address was created? or the active key was changed?


### getPublicKeys

API returns the public keys managed by the wallet. The wallet can limit the number of public keys to return. The wallet might use many public keys internally. However, it returns 2-3 public keys in some cases.
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarify, why does it return 2-3 in some cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The wallet can limit the number of public keys to return. The wallet can manage many public keys internally; however, choose few of them to return for privacy reasons.
I've updated this part.

@mariano54
Copy link
Contributor

Can we add as a requirement, that all major browsers are supported?
Thanks.

@dimitrysuen
Copy link
Contributor Author

Can we add as a requirement, that all major browsers are supported? Thanks.

APIs described in this CHiP don't rely on specific browser features. Currently, the extension API of the mainstream browsers is compatible with each other, and the wallet can achieve support for the mainstream browsers with few adjustments. However, different browser web stores have different review requirements and permission restrictions, so it may be better to let the wallet developers decide on the browser support.

@ChiaMineJP
Copy link

Thanks for great work!
I'm listing my comments for this proposal.

1. What if multiple CHIP-2 compatible browser wallets installed at the same time.

A dApp needs to ensure which browser wallet it is communicating with.
And maybe an identifier of a CHIP-2 compatible browser wallet needs to be prepared like browser’s user-agent information.

2. What if an undefined method is requested?

Just throwing an exception?

3. Listener function for event uses positional arguments like

window.chia.on(“name”, function(arg1, arg2, …) {...}); .
Shouldn’t this be like this? (Object parameter)
window.chia.on(“name”, function({arg1: val, arg2: val2,...}));

4. When communicating with a dApp and a CHIP-2 compatible browser wallet, shouldn't payloads between the two be encrypted?

When communicating via window.postMessage API, it is able for 3rd party browser extensions to intercept it. Maybe it should be another CHIP because this is a matter of a lower layer.

5. Shouldn't a browser wallet detect whether a page contains dApp script?

Current CHIP-2 proposal assumes that the direction of communication is from dApp to browser wallet. Shouldn’t CHIP-2 mention the counter direction?
Like detecting existence of a dApp running in a web page.

6. The properties of a params for getPublicKeys API are both optional. Can the whole params argument be optional?

getPublicKeys(params: {limit?: number, offset?: number}): string[]
getPublicKeys(params?: {limit?: number, offset?: number}): string[]

7. Shouldn't we explicitly declare supporting versions of browsers?

In old browsers, some ES6 syntax might not work in a browser extension context. E.g. lambda function, destructuring assignments, async/await, (possibly) class. Should we specify the minimum version of supporting browsers in the CHIP-2? Or should the browser extension specification be defined in another CHIP?

8. Typos

  • sendTransactionParams => The first letter should be capitalized.
  • Shouldn’t the first letter of Errors be capitalized?
  • userRejectdRequestError => userRejectedRequestError

@dimitrysuen
Copy link
Contributor Author

@ChiaMineJP thanks for the detailed suggestions.

We've put on the modification/description about comment 1/2/3/6/8.

4. When communicating with a dApp and a CHIP-2 compatible browser wallet, shouldn't payloads between the two be encrypted?

The wallet should deal with the security of communication. However, I think it's difficult to secure the communication by encryption in the frontend. 3rd party browser extensions could decrypt the communication.

5. Shouldn't a browser wallet detect whether a page contains dApp script?

I don't think it's compulsive for the wallet to detect the dApp. The wallet can take it as an extra feature.

7. Shouldn't we explicitly declare supporting versions of browsers?

Regarding the browser version support, it should be determined by the wallet. The wallet can decide the minimum version of the supported browsers in the manifest file according to their internal implementation.

@ChiaMineJP
Copy link

Adding name property to window.chia is nice.
Plus, this is just a small suggestion but how about adding version property to window.chia ?
This is great to maintain compatibility in the future.

@dimitrysuen
Copy link
Contributor Author

dimitrysuen commented Jul 17, 2022

Adding name property to window.chia is nice. Plus, this is just a small suggestion but how about adding version property to window.chia ? This is great to maintain compatibility in the future.

Ya, I agree. I added two fields, version and apiVersion, into the wallet interface.


Sign the message encoded as a hex string using the private key associated with the public key.

The internal implementation in the wallet is `bls_sign(private_key, sha256("\x18Chia Signed Message:\n" + len(message) + message))`. To prevent replay attacks, dApps should add the current `networkId` and `timestamp` into the message. If the dApp doesn't care which chain they're on, they can include the `timestamp` only.

Choose a reason for hiding this comment

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

For len(message) values that don't fit in a single byte, is there an expected byte ordering here? Network byte order would be what I would expect.

@hoffmang9 hoffmang9 merged commit b45154f into Chia-Network:main Oct 25, 2022
CHIPs Backlog automation moved this from Draft to Accepted Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Draft
Development

Successfully merging this pull request may close these issues.

None yet