Add ownership checks the Registry dApp #4001
Conversation
| @@ -44,8 +44,8 @@ export default class Application extends Component { | |||
|
|
|||
| static propTypes = { | |||
| accounts: PropTypes.object.isRequired, | |||
| contract: nullableProptype(PropTypes.object).isRequired, | |||
| fee: nullableProptype(PropTypes.object).isRequired | |||
| contract: nullableProptype(PropTypes.object.isRequired), | |||
There was a problem hiding this comment.
Why are we swapping the isRequired here?
| @@ -94,6 +69,7 @@ class Lookup extends Component { | |||
| <MenuItem value='IMG' primaryText='IMG – hash of a picture in the blockchain' /> | |||
| <MenuItem value='CONTENT' primaryText='CONTENT – hash of a data in the blockchain' /> | |||
| <MenuItem value='reverse' primaryText='reverse – find a name for an address' /> | |||
| <MenuItem value='owner' primaryText='owner – find a the owner' /> | |||
There was a problem hiding this comment.
Can we just double-check with @gavofyork on this - there was obviously reasons why we didn't want transfers (squatting), just want to make doubly-sure on the ability to actually display the owner of anything to anybody. (Do we want it sem-opaque o fully transparent)
| const values = [ | ||
| sha3(name) | ||
| ]; | ||
| return getOwner(contract, name) |
There was a problem hiding this comment.
Happy with this part. i.e. warn up-front if an action is possible or not.
| @@ -78,35 +79,21 @@ const renderQueue = (queue) => { | |||
| class Names extends Component { | |||
|
|
|||
| static propTypes = { | |||
| error: nullableProptype(PropTypes.object.isRequired), | |||
There was a problem hiding this comment.
As per comment above, believe that nullableProptype(type).isRequired is probably more in line with intent
There was a problem hiding this comment.
It would make sense to write it that IMO, however it doesn't work. It's done like this everywhere else in the app.
There was a problem hiding this comment.
Missing:
dapps/registry/Application/application.js:47
dapps/registry/Application/application.js:48
views/Wallet/wallet.js:74
|
|
||
| const allAccounts = Object.assign({}, accounts.all, contacts); | ||
|
|
||
| // Add lower case addresses to map |
There was a problem hiding this comment.
Why is this lowerCase and not checksum-case? (bear in mind this has an impact anywhere where the address is used in icons, checksumIcon !== lowercaseIcon)
There was a problem hiding this comment.
It's just because sometimes the returned data from a contract method isn't formatted as an address. This does not change the displayed address, it only adds to the Address Map the key for the lower case addresses.
|
|
||
| return api | ||
| .eth | ||
| .getStorageAt(address, position) |
There was a problem hiding this comment.
Would rather do lowerCase here, instead of global to app.
| } | ||
|
|
||
| onKeyDown = (event) => { | ||
| if (event.which !== 13) { |
There was a problem hiding this comment.
You might want to use import keycode from 'keycode', which you used in the address selector.
| } | ||
| }; | ||
| } | ||
|
|
||
| const mapStateToProps = (state) => ({ ...state.names, fee: state.fee }); | ||
| const mapDispatchToProps = (dispatch) => bindActionCreators({ reserve, drop }, dispatch); | ||
| const mapDispatchToProps = (dispatch) => bindActionCreators({ clearError, reserve, drop }, dispatch); |
There was a problem hiding this comment.
Would actually prefer multiple lines now. :)
| import postTx from '../util/post-tx'; | ||
|
|
||
| export const clearError = () => ({ |
There was a problem hiding this comment.
If clearError is getting handled app-wide (in dapps/registry/reducers), why not declare it in dapps/registry/actions to DRY the code?
| Object | ||
| .keys(allAccounts) | ||
| .forEach((address) => { | ||
| allAccounts[address.toLowerCase()] = allAccounts[address]; |
There was a problem hiding this comment.
[tiniest grumble] This could be Object.values(…).reduce.
|
Looks mostly good to me! |
jacogr
added
A8-looksgood 🦄
This PR closes #3037
Add proper methods to get the owner of a name in the Registry.
Use it in name lookups, reserving, resolving, etc...
Add a new lookup query: get the owner of a name.