From 570fc56af71f8fde7f22a2558a90f3e365a22324 Mon Sep 17 00:00:00 2001 From: Amaury Martiny Date: Tue, 21 Nov 2017 15:50:38 +0100 Subject: [PATCH] Optimize & group dapp requests (#7083) * Group similar methods in same grouping * Add a shell_getMethodGroups API * Small code clean changes * Fix bug dapp.name not showing * Additional error handling * Store sources in own map * Remove observable variables where not needed * Refactor code and fix bug dapp not showing on approve --- js/src/DappRequests/README.md | 9 + js/src/DappRequests/Request/request.js | 85 ----- .../RequestGroups/RequestGroups.css | 39 ++ .../RequestGroups/RequestGroups.js | 116 ++++++ .../{Request => RequestGroups}/index.js | 2 +- js/src/DappRequests/dappRequests.css | 17 - js/src/DappRequests/dappRequests.js | 53 +-- .../{filteredRequests.js => methodGroups.js} | 25 +- js/src/DappRequests/store.js | 340 +++++++++--------- js/src/shellMiddleware.js | 41 ++- 10 files changed, 404 insertions(+), 323 deletions(-) create mode 100644 js/src/DappRequests/README.md delete mode 100644 js/src/DappRequests/Request/request.js create mode 100644 js/src/DappRequests/RequestGroups/RequestGroups.css create mode 100644 js/src/DappRequests/RequestGroups/RequestGroups.js rename js/src/DappRequests/{Request => RequestGroups}/index.js (94%) rename js/src/DappRequests/{filteredRequests.js => methodGroups.js} (78%) diff --git a/js/src/DappRequests/README.md b/js/src/DappRequests/README.md new file mode 100644 index 000000000..f4ed60b40 --- /dev/null +++ b/js/src/DappRequests/README.md @@ -0,0 +1,9 @@ +# Terminology used + +To be clear with the terminology used in the code here: + +- a *method* is an allowed JSON-RPC api method or a shell method +- a *methodGroup* is the grouping of similar methods (see `methodGroups.js`) +- a *permission* is a boolean which tells if an app is allowed to call a method or not +- a *request* is when an app prompts the shell to call a method +- a *requestGroup* is an array of *requests* whose methods are in the same *methodGroup* diff --git a/js/src/DappRequests/Request/request.js b/js/src/DappRequests/Request/request.js deleted file mode 100644 index 776fd9089..000000000 --- a/js/src/DappRequests/Request/request.js +++ /dev/null @@ -1,85 +0,0 @@ -// Copyright 2015-2017 Parity Technologies (UK) Ltd. -// This file is part of Parity. - -// Parity is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. - -// Parity is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. - -// You should have received a copy of the GNU General Public License -// along with Parity. If not, see . - -import React from 'react'; -import PropTypes from 'prop-types'; -import { FormattedMessage } from 'react-intl'; - -import Button from '@parity/ui/lib/Button'; - -import DappsStore from '@parity/shared/lib/mobx/dappsStore'; - -export default function Request ({ appId, className, approveRequest, denyRequest, queueId, request: { from, method } }) { - const _onApprove = () => approveRequest(queueId, false); - const _onApproveAll = () => approveRequest(queueId, true); - const _onReject = () => denyRequest(queueId); - - const app = DappsStore.get().getAppById(appId); - - return ( -
- -
-
-
- ); -} - -Request.propTypes = { - appId: PropTypes.string.isRequired, - className: PropTypes.string, - approveRequest: PropTypes.func.isRequired, - denyRequest: PropTypes.func.isRequired, - queueId: PropTypes.number.isRequired, - request: PropTypes.object.isRequired -}; diff --git a/js/src/DappRequests/RequestGroups/RequestGroups.css b/js/src/DappRequests/RequestGroups/RequestGroups.css new file mode 100644 index 000000000..65335e85e --- /dev/null +++ b/js/src/DappRequests/RequestGroups/RequestGroups.css @@ -0,0 +1,39 @@ +/* Copyright 2015-2017 Parity Technologies (UK) Ltd. +/* This file is part of Parity. +/* +/* Parity is free software: you can redistribute it and/or modify +/* it under the terms of the GNU General Public License as published by +/* the Free Software Foundation, either version 3 of the License, or +/* (at your option) any later version. +/* +/* Parity is distributed in the hope that it will be useful, +/* but WITHOUT ANY WARRANTY; without even the implied warranty of +/* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +/* GNU General Public License for more details. +/* +/* You should have received a copy of the GNU General Public License +/* along with Parity. If not, see . +*/ + +$backgroundOne: #f80; +$backgroundTwo: #e57a00; + +.requestGroups { + background: $backgroundOne; + background: linear-gradient($backgroundOne, $backgroundTwo); + padding: 0.5em; + text-align: right; + color: white; + + > span { + margin-right: 30px; + } + + .requestGroup { + margin-top: 2px; + + .requestGroupTitle { + margin-right: 10px; + } + } +} diff --git a/js/src/DappRequests/RequestGroups/RequestGroups.js b/js/src/DappRequests/RequestGroups/RequestGroups.js new file mode 100644 index 000000000..42328ed2a --- /dev/null +++ b/js/src/DappRequests/RequestGroups/RequestGroups.js @@ -0,0 +1,116 @@ +// Copyright 2015-2017 Parity Technologies (UK) Ltd. +// This file is part of Parity. + +// Parity is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Parity is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Parity. If not, see . + +import React, { PureComponent } from 'react'; +import PropTypes from 'prop-types'; +import { FormattedMessage } from 'react-intl'; + +import Popup from 'semantic-ui-react/dist/commonjs/modules/Popup'; +import Button from '@parity/ui/lib/Button'; + +import DappsStore from '@parity/shared/lib/mobx/dappsStore'; + +import styles from './RequestGroups.css'; + +export default class RequestGroups extends PureComponent { + state = { + opened: false + }; + + handleApproveRequestGroup = groupId => { + const { requestGroups, onApproveRequestGroup } = this.props; + + onApproveRequestGroup(Object.values(requestGroups[groupId].map(({ requestId }) => requestId))); + } + + handleRejectRequestGroup = groupId => { + const { requestGroups, onRejectRequestGroup } = this.props; + + onRejectRequestGroup(Object.values(requestGroups[groupId].map(({ requestId }) => requestId))); + } + + renderPopupContent = groupId => { + const { requestGroups } = this.props; + // Get unique list of methods in that request group + const requestedMethods = [...new Set( + Object.values(requestGroups[groupId]) + .map(request => request.data.method || request.data.params[0]) + )]; + + return `Requested methods: ${requestedMethods.join(', ')}`; + } + + render () { + const { + appId, + requestGroups + } = this.props; + + const app = DappsStore.get().getAppById(appId); + + return ( +
+ + {Object.keys(requestGroups).map(groupId => ( +
+ + Permission for{' '} + {groupId} } + content={ this.renderPopupContent(groupId) } + /> + +
+ ))} +
+ ); + } +} + +RequestGroups.propTypes = { + appId: PropTypes.string.isRequired, + className: PropTypes.string, + onApproveRequestGroup: PropTypes.func.isRequired, + onRejectRequestGroup: PropTypes.func.isRequired, + requestGroups: PropTypes.object.isRequired +}; diff --git a/js/src/DappRequests/Request/index.js b/js/src/DappRequests/RequestGroups/index.js similarity index 94% rename from js/src/DappRequests/Request/index.js rename to js/src/DappRequests/RequestGroups/index.js index 20fb91088..af5e54d12 100644 --- a/js/src/DappRequests/Request/index.js +++ b/js/src/DappRequests/RequestGroups/index.js @@ -14,4 +14,4 @@ // You should have received a copy of the GNU General Public License // along with Parity. If not, see . -export default from './request'; +export default from './RequestGroups'; diff --git a/js/src/DappRequests/dappRequests.css b/js/src/DappRequests/dappRequests.css index 8d82bfa1b..673a7b33d 100644 --- a/js/src/DappRequests/dappRequests.css +++ b/js/src/DappRequests/dappRequests.css @@ -15,27 +15,10 @@ /* along with Parity. If not, see . */ -$backgroundOne: #f80; -$backgroundTwo: #e57a00; - .requests { - color: white; position: fixed; left: 0; right: 0; bottom: 0; z-index: 1001; /* sits above sync warning */ - - .request { - align-items: center; - background: $backgroundOne; - background: linear-gradient($backgroundOne, $backgroundTwo); - display: flex; - justify-content: flex-end; - padding: 0.5em; - - > span { - margin-right: 1em; - } - } } diff --git a/js/src/DappRequests/dappRequests.js b/js/src/DappRequests/dappRequests.js index b6acb774e..263063d5e 100644 --- a/js/src/DappRequests/dappRequests.js +++ b/js/src/DappRequests/dappRequests.js @@ -15,36 +15,43 @@ // along with Parity. If not, see . import { observer } from 'mobx-react'; -import React from 'react'; +import React, { PureComponent } from 'react'; -import Request from './Request'; +import RequestGroups from './RequestGroups'; import Store from './store'; import styles from './dappRequests.css'; -function DappRequests () { - const store = Store.get(); +class DappRequests extends PureComponent { + store = Store.get(); - if (!store || !store.hasRequests) { - return null; + handleApproveRequestGroup = requestIds => { + requestIds.forEach(this.store.approveRequest); } - return ( -
- { - store.squashedRequests.map(({ appId, queueId, request: { data } }) => ( - - )) - } -
- ); + handleRejectRequestGroup = requestIds => { + requestIds.forEach(this.store.rejectRequest); + } + + render () { + if (!this.store || !this.store.hasRequests) { + return null; + } + + return ( +
+ {Object.keys(this.store.groupedRequests) + .map(appId => ( + + ))} +
+ ); + } } export default observer(DappRequests); diff --git a/js/src/DappRequests/filteredRequests.js b/js/src/DappRequests/methodGroups.js similarity index 78% rename from js/src/DappRequests/filteredRequests.js rename to js/src/DappRequests/methodGroups.js index 743660428..c37357428 100644 --- a/js/src/DappRequests/filteredRequests.js +++ b/js/src/DappRequests/methodGroups.js @@ -14,21 +14,19 @@ // You should have received a copy of the GNU General Public License // along with Parity. If not, see . -export default { +const methodGroups = { shell: { methods: [ 'shell_getApps', 'shell_getFilteredMethods', + 'shell_getMethodGroups', 'shell_getMethodPermissions', 'shell_setAppVisibility', 'shell_setMethodPermissions' ] }, accountsView: { - methods: [ - 'parity_accountsInfo', - 'parity_allAccountsInfo' - ] + methods: ['parity_accountsInfo', 'parity_allAccountsInfo'] }, accountsCreate: { methods: [ @@ -42,10 +40,7 @@ export default { ] }, accountsEdit: { - methods: [ - 'parity_setAccountName', - 'parity_setAccountMeta' - ] + methods: ['parity_setAccountName', 'parity_setAccountMeta'] }, upgrade: { methods: [ @@ -76,3 +71,15 @@ export default { ] } }; + +const methodGroupFromMethod = {}; // Maps method to methodGroup + +// Populate methodGroupFromMethod +Object.keys(methodGroups).forEach(groupId => { + methodGroups[groupId].methods.forEach(method => { + methodGroupFromMethod[method] = groupId; + }); +}); + +export { methodGroupFromMethod }; +export default methodGroups; diff --git a/js/src/DappRequests/store.js b/js/src/DappRequests/store.js index 4c5448a61..5a89ad29a 100644 --- a/js/src/DappRequests/store.js +++ b/js/src/DappRequests/store.js @@ -19,19 +19,17 @@ import store from 'store'; import { sha3 } from '@parity/api/lib/util/sha3'; -import filteredRequests from './filteredRequests'; +import { methodGroupFromMethod } from './methodGroups'; const LS_PERMISSIONS = '_parity::dapps::methods'; -let nextQueueId = 0; - export default class Store { - @observable permissions = {}; - @observable requests = []; - @observable tokens = {}; + @observable requests = {}; // Maps requestId to request middleware = []; - sources = {}; + permissions = {}; // Maps `${method}:${appId}` to true/false + sources = {}; // Maps requestId to a postMessage source + tokens = {}; // Maps token to appId constructor (provider) { this.provider = provider; @@ -41,131 +39,112 @@ export default class Store { } @computed get hasRequests () { - return this.requests.length !== 0; + return Object.keys(this.requests).length !== 0; } - @computed get squashedRequests () { - const duplicates = {}; + @computed get groupedRequests () { + // Group by appId on top level, and by methodGroup on 2nd level + return Object.keys(this.requests).reduce((accumulator, requestId) => { + const { data } = this.requests[requestId]; + const appId = this.tokens[data.token]; + const method = this.getMethodFromRequest(requestId); + const methodGroup = methodGroupFromMethod[method]; // Get the methodGroup the current request belongs to - return this.requests.filter(({ request: { data: { method, token } } }) => { - const section = this.getFilteredSectionName(method); - const id = `${token}:${section}`; + accumulator[appId] = accumulator[appId] || {}; + accumulator[appId][methodGroup] = accumulator[appId][methodGroup] || []; + accumulator[appId][methodGroup].push({ data, requestId }); // Append the requestId field in the request object - if (!duplicates[id]) { - duplicates[id] = true; - return true; - } - - return false; - }); + return accumulator; + }, {}); } - @action createToken = (appId) => { - const token = sha3(`${appId}:${Date.now()}`); + @action queueRequest = (requestId, { data, source }) => { + this.sources[requestId] = source; + // Create a new this.requests object to update mobx store + this.requests = { + ...this.requests, + [requestId]: { data } + }; + }; - this.tokens = Object.assign({}, this.tokens, { - [token]: appId - }); - - return token; - } - - @action removeRequest = (_queueId) => { - this.requests = this.requests.filter(({ queueId }) => queueId !== _queueId); - delete this.sources[_queueId]; - } - - @action queueRequest = (request) => { - const { data, origin, source } = request; + @action approveRequest = requestId => { + const { data } = this.requests[requestId]; + const method = this.getMethodFromRequest(requestId); const appId = this.tokens[data.token]; + const source = this.sources[requestId]; - let queueId = ++nextQueueId; - - this.sources[queueId] = source; - this.requests = this.requests.concat([{ - appId, - queueId, - request: { - data, - origin - } - }]); - } - - @action addTokenPermission = (method, token) => { - const id = `${method}:${this.tokens[token]}`; - - this.permissions = Object.assign({}, this.permissions, { - [id]: true - }); - this.savePermissions(); - } - - @action approveSingleRequest = ({ queueId, request: { data } }) => { - const source = this.sources[queueId]; - - this.removeRequest(queueId); + this.addAppPermission(method, appId); + this.removeRequest(requestId); if (data.api) { this.executePubsubCall(data, source); } else { this.executeMethodCall(data, source); } - } + }; - @action approveRequest = (queueId, approveAll) => { - const queued = this.findRequest(queueId); + @action rejectRequest = requestId => { + const { data } = this.requests[requestId]; + const source = this.sources[requestId]; - if (approveAll) { - const { request: { data: { method, token, params } } } = queued; - - this.getFilteredSection(method || params[0]).methods.forEach((m) => { - this.addTokenPermission(m, token); - this.findMatchingRequests(m, token).forEach(this.approveSingleRequest); - }); - } else { - this.approveSingleRequest(queued); - } - } - - @action rejectRequest = (queueId) => { - const { request: { data } } = this.findRequest(queueId); - const source = this.sources[queueId]; - - this.removeRequest(queueId); + this.removeRequest(requestId); this.rejectMessage(source, data); + }; + + @action removeRequest = requestId => { + delete this.requests[requestId]; + delete this.sources[requestId]; + + // Create a new object to update mobx store + this.requests = { ...this.requests }; + }; + + getPermissionId = (method, appId) => `${method}:${appId}` // Create an id to identify permissions based on method and appId + + getMethodFromRequest = requestId => { + const { data: { method, params } } = this.requests[requestId]; + + return method || params[0]; } - @action rejectMessage = (source, { id, from, method, token }) => { + rejectMessage = (source, { id, from, method, token }) => { if (!source) { return; } - source.postMessage({ - error: `Method ${method} not allowed`, - id, - from: 'shell', - result: null, - to: from, - token - }, '*'); - } + source.postMessage( + { + error: `Method ${method} not allowed`, + id, + from: 'shell', + result: null, + to: from, + token + }, + '*' + ); + }; - @action setPermissions = (_permissions) => { + addAppPermission = (method, appId) => { + this.permissions[this.getPermissionId(method, appId)] = true; + this.savePermissions(); + }; + + setPermissions = _permissions => { const permissions = {}; - Object.keys(_permissions).forEach((id) => { + Object.keys(_permissions).forEach(id => { permissions[id] = !!_permissions[id]; }); - this.permissions = Object.assign({}, this.permissions, permissions); + this.permissions = permissions; this.savePermissions(); return true; - } + }; addMiddleware (middleware) { - if (!middleware || (typeof middleware !== 'function')) { + if (!middleware || typeof middleware !== 'function') { throw new Error('Interceptor middleware does not implement a function'); } @@ -174,114 +153,131 @@ export default class Store { return true; } + createToken = appId => { + const token = sha3(`${appId}:${Date.now()}`); + + this.tokens[token] = appId; + return token; + }; + hasValidToken = (method, appId, token) => { if (!token) { return method === 'shell_requestNewToken'; } return this.tokens[token] === appId; - } + }; hasTokenPermission = (method, token) => { return this.hasAppPermission(method, this.tokens[token]); - } + }; hasAppPermission = (method, appId) => { - return this.permissions[`${method}:${appId}`] || false; - } + return !!this.permissions[this.getPermissionId(method, appId)]; + }; savePermissions = () => { store.set(LS_PERMISSIONS, this.permissions); - } - - findRequest (_queueId) { - return this.requests.find(({ queueId }) => queueId === _queueId); - } - - findMatchingRequests (_method, _token) { - return this.requests.filter(({ request: { data: { method, token, params } } }) => (method === _method || (params && params[0] === _method)) && token === _token); - } + }; _methodCallbackPost = (id, from, source, token) => { return (error, result) => { if (!source) { return; } - - source.postMessage({ - error: error - ? error.message - : null, - id, - from: 'shell', - to: from, - result, - token - }, '*'); + source.postMessage( + { + error: error ? error.message : null, + id, + from: 'shell', + to: from, + result, + token + }, + '*' + ); }; - } + }; executePubsubCall = ({ api, id, from, token, params }, source) => { const callback = this._methodCallbackPost(id, from, source, token); - this.provider - .subscribe(api, callback, params) - .then((result, error) => { - this._methodCallbackPost(id, from, source, token)(null, result); - }); - } + this.provider.subscribe(api, callback, params).then((result, error) => { + this._methodCallbackPost(id, from, source, token)(null, result); + }); + }; executeMethodCall = ({ id, from, method, params, token }, source) => { - const callback = this._methodCallbackPost(id, from, source, token); - const isHandled = this.middleware.find((middleware) => middleware(from, method, params, callback)); + try { + const callback = this._methodCallbackPost(id, from, source, token); + const isHandled = this.middleware.some(middleware => { + try { + return middleware(from, method, params, callback); + } catch (error) { + console.error(`Middleware error handling '${method}'`, error); + } - if (!isHandled) { - this.provider.send(method, params, callback); + return false; + }); + + if (!isHandled) { + this.provider.send(method, params, callback); + } + } catch (error) { + console.error(`Execution error handling '${method}'`, error); } - } + }; - getFilteredSectionName = (method) => { - return Object.keys(filteredRequests).find((key) => { - return filteredRequests[key].methods.includes(method); - }); - } + receiveMessage = ({ data, source }) => { + try { + if (!data) { + return; + } - getFilteredSection = (method) => { - return filteredRequests[this.getFilteredSectionName(method)]; - } + const { from, method, to, token, params, api, subId, id } = data; - receiveMessage = ({ data, origin, source }) => { - if (!data) { - return; + if (to !== 'shell' || !from || from === 'shell') { + return; + } + + if (!this.hasValidToken(method, from, token)) { + this.rejectMessage(source, data); + return; + } + + if ( + (method && + methodGroupFromMethod[method] && + !this.hasTokenPermission(method, token)) || + (api && + methodGroupFromMethod[params[0]] && + !this.hasTokenPermission(method, token)) + ) { + this.queueRequest(id, { // The requestId of a request is the id inside data + data, + source + }); + return; + } + + if (api) { + this.executePubsubCall(data, source); + } else if (subId) { + const unsubscribePromise = subId === '*' + ? this.provider.unsubscribeAll() + : this.provider.unsubscribe(subId); + + unsubscribePromise + .then(v => + this._methodCallbackPost(id, from, source, token)(null, v) + ); + } else { + this.executeMethodCall(data, source); + } + } catch (error) { + console.error('Exception handling data', data, error); } - - const { from, method, to, token, params, api, subId, id } = data; - - if (to !== 'shell' || !from || from === 'shell') { - return; - } - - if (!this.hasValidToken(method, from, token)) { - this.rejectMessage(source, data); - return; - } - - if ((method && this.getFilteredSection(method) && !this.hasTokenPermission(method, token)) || - (api && this.getFilteredSection(params[0]) && !this.hasTokenPermission(method, token))) { - this.queueRequest({ data, origin, source }); - return; - } - - if (api) { - this.executePubsubCall(data, source); - } else if (subId) { - subId === '*' - ? this.provider.unsubscribeAll().then(v => this._methodCallbackPost(id, from, source, token)(null, v)) - : this.provider.unsubscribe(subId).then(v => this._methodCallbackPost(id, from, source, token)(null, v)); - } else { - this.executeMethodCall(data, source); - } - } + }; static instance = null; diff --git a/js/src/shellMiddleware.js b/js/src/shellMiddleware.js index 7bdb9d259..a8f1e31bc 100644 --- a/js/src/shellMiddleware.js +++ b/js/src/shellMiddleware.js @@ -14,13 +14,14 @@ // You should have received a copy of the GNU General Public License // along with Parity. If not, see . +import mobx from 'mobx'; import flatten from 'lodash.flatten'; import { sha3 } from '@parity/api/lib/util/sha3'; import VisibleStore from '@parity/shared/lib/mobx/dappsStore'; import RequestStore from './DappRequests/store'; -import filteredRequests from './DappRequests/filteredRequests'; +import methodGroups from './DappRequests/methodGroups'; export default function execute (appId, method, params, callback) { const visibleStore = VisibleStore.get(); @@ -30,29 +31,35 @@ export default function execute (appId, method, params, callback) { case 'shell_getApps': const [displayAll] = params; - callback(null, displayAll - ? visibleStore.allApps.slice() - : visibleStore.visibleApps.slice() + callback( + null, + displayAll + ? visibleStore.allApps.slice().map(mobx.toJS) + : visibleStore.visibleApps.slice().map(mobx.toJS) ); return true; case 'shell_getFilteredMethods': - callback(null, flatten( - Object - .keys(filteredRequests) - .map((key) => filteredRequests[key].methods) - )); + callback( + null, + flatten(Object.keys(methodGroups).map(key => methodGroups[key].methods)) + ); + return true; + + case 'shell_getMethodGroups': + callback( + null, + methodGroups + ); return true; case 'shell_getMethodPermissions': - callback(null, requestStore.permissions); + callback(null, mobx.toJS(requestStore.permissions)); return true; case 'shell_loadApp': const [_loadId, loadParams] = params; - const loadId = _loadId.substr(0, 2) !== '0x' - ? sha3(_loadId) - : _loadId; + const loadId = _loadId.substr(0, 2) !== '0x' ? sha3(_loadId) : _loadId; const loadUrl = `/${loadId}/${loadParams || ''}`; window.location.hash = loadUrl; @@ -67,9 +74,11 @@ export default function execute (appId, method, params, callback) { case 'shell_setAppVisibility': const [changeId, visibility] = params; - callback(null, visibility - ? visibleStore.showApp(changeId) - : visibleStore.hideApp(changeId) + callback( + null, + visibility + ? visibleStore.showApp(changeId) + : visibleStore.hideApp(changeId) ); return true;