Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Visible accounts for dapps (default whitelist) #3898

Merged
merged 23 commits into from
Dec 27, 2016
Merged

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Dec 19, 2016

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Dec 19, 2016
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Dec 20, 2016
@jacogr
Copy link
Contributor Author

jacogr commented Dec 20, 2016

There is a massive issue here though - none of our internal dapps use eth_accounts so it doesn't reflect only the accounts it is supposed to see... All dapps needs to be updated to use a combination of eth_accounts & parity_accountsInfo again.

@jacogr jacogr changed the title Visible accounts for dapps (whitelist) Visible accounts for dapps (default whitelist) Dec 20, 2016
@gavofyork
Copy link
Contributor

gavofyork commented Dec 20, 2016

i don't understand - does one of eth_accounts and parity_accountsInfo show more accounts than just the restricted set that the dapp is allowed to see?

@gavofyork gavofyork modified the milestone: 1.5 Tenuity Dec 20, 2016
@jacogr
Copy link
Contributor Author

jacogr commented Dec 20, 2016

It seems like parity_accounts returns everything. Dapps don't currently use eth_accounts at all. (Whitelist is set, verified by looking at dapps_policy.json), eg.

$ cat ~/Library/Application\ Support/io.parity.ethereum/keys/test/dapps_policy.json
{"default":{"Whitelist":["0xf6abb80f11f269e4500a05721680e0a3ab075ecf"]}}% 

Registry dapp still shows all accounts. However using the Js console -

parity.api.eth.accounts().then(console.log)
<
undefined
 
0xF6ABb80F11f269e4500A05721680E0a3AB075Ecf

&&

parity.api.parity.accounts().then((accs) => console.log(Object.keys(accs).filter((addr) => accs[addr].uuid).length))
<
undefined
 
13

@tomusdrw
Copy link
Collaborator

parity_accounts or parity_accountsInfo shouldn't be available anywhere except for IPC and Signer interfaces so it's never exposed to standard dapps.

@jacogr
Copy link
Contributor Author

jacogr commented Dec 21, 2016

The built-in dapps do use parity_accounts (and it returns everything) - the reason they all use this call is to have access to the account names.

@jacogr
Copy link
Contributor Author

jacogr commented Dec 21, 2016

Dapp issues & accountsInfo/allAccountsInfo addressed in https://github.com/ethcore/parity/pull/3931

@gavofyork
Copy link
Contributor

needs resolving

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 85.536% when pulling be7458e on jg-dapps-accounts into 63d68aa on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.539% when pulling be7458e on jg-dapps-accounts into 63d68aa on master.

@gavofyork
Copy link
Contributor

needs resolving again. @ngotchac / @derhuerst a review would be good, too...

# Conflicts:
#	js/src/ui/Icons/index.js
@ngotchac ngotchac added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. A0-pleasereview 🤓 Pull request needs code review. and removed A0-pleasereview 🤓 Pull request needs code review. A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Dec 27, 2016
@@ -17,6 +17,14 @@
import { inAddress, inData, inHex, inNumber16, inOptions } from '../../format/input';
import { outAccountInfo, outAddress, outChainStatus, outHistogram, outNumber, outPeers, outTransaction } from '../../format/output';

function inAddresses (addresses) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't this in format/input ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I initially moved it to the top to get going (testing), but never moved it to the formatters as it should be. (Nor really major since it is the only place it is used, but still not consistent.)

return (addresses || []).map(inAddress);
}

function outAddresses (addresses) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't this in format/output ?

@ngotchac ngotchac added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 27, 2016
@ngotchac
Copy link
Contributor

Looks good. Small grumble, also the Todo comment for the Modal could be implemented now that the new address select component is in Master, but I don't think that we want to do this now.

@ngotchac ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Dec 27, 2016
@gavofyork gavofyork merged commit b27c809 into master Dec 27, 2016
@gavofyork gavofyork deleted the jg-dapps-accounts branch December 27, 2016 15:23
@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 85.079% when pulling 4df87af on jg-dapps-accounts into 80eae8c on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 85.079% when pulling 4df87af on jg-dapps-accounts into 80eae8c on master.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants