Add wallet_getAddressBook JSON-RPC method #2253

loredanacirstea · 2019-08-27T16:33:07Z

Add wallet_getAddressBook JSON-RPC method

LefterisJP · 2019-08-27T17:48:00Z

Nice inititative! Would it also make sense to extend this to also include tokens watched (owned and cared for by the owner) for each address.

This is something that many apps including Rotki would very very much appreciate.

loredanacirstea · 2019-08-27T17:52:25Z

@LefterisJP , this was exactly the plan! Glad to see it is needed.

karalabe · 2019-08-27T17:58:58Z

The field filter seems weird on RPC. That's essentially what GraphQL is for and how it works. I'm not sure it's a good idea to start introducing GraphQL concepts into the RPC.

Also, @holiman has been working on Clef and it's external API to solve all the account management problems securely. I'd much rather standardize that than reinvent everything yet again. https://github.com/ethereum/go-ethereum/tree/master/cmd/clef

As suggested in https://ethereum-magicians.org/t/eip-2253-add-wallet-getaddressbook-json-rpc-method/3592/6

loredanacirstea · 2019-09-03T06:36:42Z

@karalabe,
@holiman said that this EIP is a good start:
https://ethereum-magicians.org/t/eip-2253-add-wallet-getaddressbook-json-rpc-method/3592/16. In the discussion thread, it was clarified that you both misunderstood the purpose of this EIP initially. So, can you fix your above comment, so reviewers can merge this PR?

holiman · 2019-09-03T08:07:17Z

EIPS/eip-2253.md

+
+**Arguments:**
+- filter object, optional:
+  - `count` - type `uint256`, the number of address book records requested; optional - when missing, the entire address book is requested


I'm not sure about this. If there is a count at all, then it needs (?) to be an offset as well right? Otherwise you get this weird case where you can either get the (first?) ten, or the full thing, but not items 10 to 20.

I was thinking of count not as a filter with limit, offset, but as how many addresses are expected by the dApp. The user would control which addresses are selected and the wallet would control what/how/how many addresses are presented for selection (based on the user's data).

E.g. maybe the dApp only requires one such address - not the entire address book. Or 3 addresses - a dApp that says: you can send our asset for free to 3 friends! -> the wallet shows that you can select up to three addresses, so you can select 1, 2 or 3 addresses. (In EIP-2256 I mention: the user can amend the request by lowering the number of owned assets returned to the dApp; I see I did not add this idea here)

This said, count clearly overlaps with the already known filter concepts. So, I am open to other names. Maybe:

replace filter object with options object

replace count with limit or maxNumber

done:
s/filter/options/
s/count/limit/

I added the mention:
"It remains to be discussed if the request parameters need to specify optional and required fields or minimum and maximum number of items to be returned."

holiman · 2019-09-03T08:15:39Z

EIPS/eip-2253.md

+  "method": "wallet_getAddressBook",
+  "params": [
+    {
+      "count": 1,


This is an ambiguous IMO. What does "one entry" mean, the first one (according to what ordering?), or just some arbitrary entry?

Explained in https://github.com/ethereum/EIPs/pull/2253/files#r320375982 - I will amend the spec after I get your opinion on this.

holiman · 2019-09-03T08:15:56Z

EIPS/eip-2253.md

+
+If an address book selection is requested, the user will have to select the items from the address book list.
+
+The wallet will also show radio options for which `WalletAddressBookItem` optional fields the user wants to expose, if those fields were requested.


Suggested change

The wallet will also show radio options for which `WalletAddressBookItem` optional fields the user wants to expose, if those fields were requested.

The wallet _should_ also show radio options for which `WalletAddressBookItem` optional fields the user wants to expose, if those fields were requested.

holiman · 2019-09-03T08:19:08Z

EIPS/eip-2253.md

+
+If the entire address book is requested, the total number of address book items will be shown. However, even in this case, the user should be able to make a selection and restrict exposed fields.
+
+If an address book selection is requested, the user will have to select the items from the address book list.


Would it make sense to have a "reason" in the request? So that the backend can tell the user

A request has come in from xxx to see 1 account, with reason: "select a friend to invite"`
Approve?

This was also proposed by @danfinlay :

One parameter that might go well with this (and any permission, actually) would be a justification field

At that point, my answer was:

I understand the justification field would be in the permissions proposal. In which case, I agree. If the wallet_getAddressBook is used by the dApp (outside the permissions system), it is probably triggered by a user’s action and the dApp UI should provide the justification.

But I rethought this and you both are right. Regardless of the dApp's UI, it is reassuring for the user to see that the reason/justification on the wallet matches what the dApp shows.

reason/justification/message seem good - not sure which has been more used in other EIPs, with the same meaning.

justification was added.

EIPS/eip-2253.md

Kaykutee · 2019-09-05T10:14:20Z

Thanks a good idea. Agreed.

On Tue, Sep 3, 2019 at 12:54 PM Loredana Cirstea ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In EIPS/eip-2253.md <#2253 (comment)>: > + +## Motivation + +Address books for wallets are becoming a standard feature, greatly improving usability, while maintaining user ownership on this data. + +There is no standardized way for a dApp to leverage this information, with the user's permission. dApps can use the `wallet_getAddressBook` JSON-RPC method to retrieve strictly the information that they need from a wallet, in order to process the request. + +The current proposal will avoid duplication of effort - dApps will not need to ask the user to create another address book with them. And it also protects the user's privacy, while increasing usability. + +## Specification + +New JSON-RPC method: `wallet_getAddressBook`. + +**Arguments:** +- filter object, optional: + - `count` - type `uint256`, the number of address book records requested; optional - when missing, the entire address book is requested I was thinking of count not as a filter with limit, offset, but as how many addresses are expected by the dApp. The user would control which addresses are selected and the wallet would control what/how/how many addresses are presented for selection (based on the user's data). E.g. maybe the dApp only requires one such address - not the entire address book. Or 3 addresses - a dApp that says: you can send our asset for free to 3 friends! -> the wallet shows that you can select up to three addresses, so you can select 1, 2 or 3 addresses. (In EIP-2256 <#2256> I mention: the user can amend the request by lowering the number of owned assets returned to the dApp; I see I did not add this idea here) This said, count clearly overlaps with the already known filter concepts. So, I am open to other names. Maybe: - replace filter object with options object - replace count with limit or maxNumber — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2253?email_source=notifications&email_token=AIMOFTJFN2MG6X44URHR6U3QH2JERA5CNFSM4IQGRNBKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCDQDQYQ#discussion_r320375982>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIMOFTJ6JF57EN7SUTRUZQDQH2JERANCNFSM4IQGRNBA> .

-- Kay Gardner

- add `justification` field in the request arguments, as suggested in ethereum#2256 (comment) - replace `count` with `limit`, as `count` is more ambiguous (as revealed in ethereum#2253 (comment))

As suggested in ethereum#2253 (comment)

- add a `justification` field, as discussed in ethereum#2253 (comment) - replace `count` with `limit`, as `count` does not have the intended meaning (ethereum#2253 (comment)) - other fixes

* Add ERC for wallet_getOwnedTokens JSON-RPC method * Add EIP number & discussions URL to EIP-2256 * Fix wallet_getOwnedTokens input arguments * Add chainId as argument to wallet_getOwnedTokens As suggested in #2256 (comment) * Rename EIP-2256 method to wallet_getOwnedAssets, align data struct with EIP-747 Based on the discussions up to and including this comment: https://ethereum-magicians.org/t/eip-2256-add-wallet-getownedassets-json-rpc-method/3600/15 - rename `wallet_getOwnedTokens` to `wallet_getOwnedAssets` - use `options` in the response field, as EIP-747 proposes, to allow for different asset-specific information to be returned - use `asset` instead of `token` where generalization is needed - replace `interface` with `type`, to align with EIP-747 - add `types` as a filter field - add `decimals` as an optional field in `options` * Fix category for EIP-2256 - use Interface As suggested in #2256 (comment) * Add justification field for EIP-2256, s/count/limit - add `justification` field in the request arguments, as suggested in #2256 (comment) - replace `count` with `limit`, as `count` is more ambiguous (as revealed in #2253 (comment))

loredanacirstea · 2019-09-06T17:39:38Z

@karalabe, can you clarify your position on this EIP, so it can be merged?

loredanacirstea · 2019-09-07T17:01:57Z

I know that core devs have enough on their plate. But @karalabe, you made the effort to say “I'd much rather standardize that than reinvent everything yet again” on an EIP that proposes a new method. So, it should only be right to take the time again to correct your statement and say that it is indeed a new proposal.
I expect the filtering will be discussed more after the draft is merged.

Keep in mind my comment from the discussion thread.

karalabe · 2019-09-07T20:14:33Z

My request was to make sure it's in sync with the work Martin was putting in. Above that, I don't care.

…

On Sat, Sep 7, 2019, 20:02 Loredana Cirstea ***@***.***> wrote: I know that core devs have enough on their plate. But @karalabe <https://github.com/karalabe>, you made the effort to say *“I'd much rather standardize that than reinvent everything yet again”* on an EIP that proposes a *new* method. So, it should only be right to take the time again to correct your statement and say that it is indeed a new proposal. I expect the filtering will be discussed more after the draft is merged. Keep in mind my comment from the discussion thread <https://ethereum-magicians.org/t/eip-2253-add-wallet-getaddressbook-json-rpc-method/3592/17> . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2253?email_source=notifications&email_token=AAA7UGNLVNIORZQLK36O7G3QIPNDHA5CNFSM4IQGRNBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6E5FXY#issuecomment-529126111>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA7UGOA6THGAP4PZ62VXHTQIPNDHANCNFSM4IQGRNBA> .

* Add ERC for wallet_getOwnedTokens JSON-RPC method * Add EIP number & discussions URL to EIP-2256 * Fix wallet_getOwnedTokens input arguments * Add chainId as argument to wallet_getOwnedTokens As suggested in ethereum/EIPs#2256 (comment) * Rename EIP-2256 method to wallet_getOwnedAssets, align data struct with EIP-747 Based on the discussions up to and including this comment: https://ethereum-magicians.org/t/eip-2256-add-wallet-getownedassets-json-rpc-method/3600/15 - rename `wallet_getOwnedTokens` to `wallet_getOwnedAssets` - use `options` in the response field, as EIP-747 proposes, to allow for different asset-specific information to be returned - use `asset` instead of `token` where generalization is needed - replace `interface` with `type`, to align with EIP-747 - add `types` as a filter field - add `decimals` as an optional field in `options` * Fix category for EIP-2256 - use Interface As suggested in ethereum/EIPs#2256 (comment) * Add justification field for EIP-2256, s/count/limit - add `justification` field in the request arguments, as suggested in ethereum/EIPs#2256 (comment) - replace `count` with `limit`, as `count` is more ambiguous (as revealed in ethereum/EIPs#2253 (comment))

* Add ERC for wallet_getOwnedTokens JSON-RPC method * Add EIP number & discussions URL to EIP-2256 * Fix wallet_getOwnedTokens input arguments * Add chainId as argument to wallet_getOwnedTokens As suggested in ethereum#2256 (comment) * Rename EIP-2256 method to wallet_getOwnedAssets, align data struct with EIP-747 Based on the discussions up to and including this comment: https://ethereum-magicians.org/t/eip-2256-add-wallet-getownedassets-json-rpc-method/3600/15 - rename `wallet_getOwnedTokens` to `wallet_getOwnedAssets` - use `options` in the response field, as EIP-747 proposes, to allow for different asset-specific information to be returned - use `asset` instead of `token` where generalization is needed - replace `interface` with `type`, to align with EIP-747 - add `types` as a filter field - add `decimals` as an optional field in `options` * Fix category for EIP-2256 - use Interface As suggested in ethereum#2256 (comment) * Add justification field for EIP-2256, s/count/limit - add `justification` field in the request arguments, as suggested in ethereum#2256 (comment) - replace `count` with `limit`, as `count` is more ambiguous (as revealed in ethereum#2253 (comment))

github-actions · 2020-09-08T16:20:53Z

There has been no activity on this pull request for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

github-actions · 2020-09-15T17:08:01Z

This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

* Add ERC for wallet_getOwnedTokens JSON-RPC method * Add EIP number & discussions URL to EIP-2256 * Fix wallet_getOwnedTokens input arguments * Add chainId as argument to wallet_getOwnedTokens As suggested in ethereum#2256 (comment) * Rename EIP-2256 method to wallet_getOwnedAssets, align data struct with EIP-747 Based on the discussions up to and including this comment: https://ethereum-magicians.org/t/eip-2256-add-wallet-getownedassets-json-rpc-method/3600/15 - rename `wallet_getOwnedTokens` to `wallet_getOwnedAssets` - use `options` in the response field, as EIP-747 proposes, to allow for different asset-specific information to be returned - use `asset` instead of `token` where generalization is needed - replace `interface` with `type`, to align with EIP-747 - add `types` as a filter field - add `decimals` as an optional field in `options` * Fix category for EIP-2256 - use Interface As suggested in ethereum#2256 (comment) * Add justification field for EIP-2256, s/count/limit - add `justification` field in the request arguments, as suggested in ethereum#2256 (comment) - replace `count` with `limit`, as `count` is more ambiguous (as revealed in ethereum#2253 (comment))

loredanacirstea added 3 commits August 27, 2019 00:26

Add wallet_getAddressBook JSON-RPC method

c651225

Add discussion link, EIP number

0f42a5c

Rename wallet_getAddressBook EIP to EIP-2253

6d373d6

EIP-2253 improve specifications style

8d9c1bb

danfinlay mentioned this pull request Aug 29, 2019

Add wallet_getAddressBook MetaMask/wallet-permissions-spec#3

Closed

Add icon response field for EIP-2253

617310c

As suggested in https://ethereum-magicians.org/t/eip-2253-add-wallet-getaddressbook-json-rpc-method/3592/6

holiman reviewed Sep 3, 2019

View reviewed changes

axic reviewed Sep 5, 2019

View reviewed changes

EIPS/eip-2253.md Outdated Show resolved Hide resolved

axic added the type: Interface label Sep 5, 2019

loredanacirstea added 2 commits September 5, 2019 16:08

Fix category for EIP-2253 - use Interface

da1b2e1

As suggested in ethereum#2253 (comment)

EIP-2253 - add justification field, s/count/limit

7d0b130

- add a `justification` field, as discussed in ethereum#2253 (comment) - replace `count` with `limit`, as `count` does not have the intended meaning (ethereum#2253 (comment)) - other fixes

jennypollack mentioned this pull request Oct 1, 2019

Contacts Manager in Settings (Design Enhancements) MetaMask/metamask-extension#7069

Open

github-actions bot added the stale label Sep 8, 2020

github-actions bot closed this Sep 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wallet_getAddressBook JSON-RPC method #2253

Add wallet_getAddressBook JSON-RPC method #2253

loredanacirstea commented Aug 27, 2019

LefterisJP commented Aug 27, 2019

loredanacirstea commented Aug 27, 2019

karalabe commented Aug 27, 2019

loredanacirstea commented Sep 3, 2019

holiman Sep 3, 2019

loredanacirstea Sep 3, 2019

loredanacirstea Sep 5, 2019

holiman Sep 3, 2019

loredanacirstea Sep 3, 2019

holiman Sep 3, 2019

holiman Sep 3, 2019

loredanacirstea Sep 3, 2019

loredanacirstea Sep 5, 2019

Kaykutee commented Sep 5, 2019 via email

loredanacirstea commented Sep 6, 2019

loredanacirstea commented Sep 7, 2019

karalabe commented Sep 7, 2019 via email

github-actions bot commented Sep 8, 2020

github-actions bot commented Sep 15, 2020


		If an address book selection is requested, the user will have to select the items from the address book list.

		The wallet will also show radio options for which `WalletAddressBookItem` optional fields the user wants to expose, if those fields were requested.

	The wallet will also show radio options for which `WalletAddressBookItem` optional fields the user wants to expose, if those fields were requested.
	The wallet _should_ also show radio options for which `WalletAddressBookItem` optional fields the user wants to expose, if those fields were requested.


		If the entire address book is requested, the total number of address book items will be shown. However, even in this case, the user should be able to make a selection and restrict exposed fields.

		If an address book selection is requested, the user will have to select the items from the address book list.

Add wallet_getAddressBook JSON-RPC method #2253

Add wallet_getAddressBook JSON-RPC method #2253

Conversation

loredanacirstea commented Aug 27, 2019

LefterisJP commented Aug 27, 2019

loredanacirstea commented Aug 27, 2019

karalabe commented Aug 27, 2019

loredanacirstea commented Sep 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kaykutee commented Sep 5, 2019 via email

loredanacirstea commented Sep 6, 2019

loredanacirstea commented Sep 7, 2019

karalabe commented Sep 7, 2019 via email

github-actions bot commented Sep 8, 2020

github-actions bot commented Sep 15, 2020