feat: Relates to #360. Only allow import from Parity Signer chain account matching current chain. ETC support #483
Conversation
…ount matching current chain. ETC support
There was a problem hiding this comment.
Thanks for the PR :)
Sneaking in the ETC compatibility in this PR is not related to #360 and it makes it hard to test and review. Considering the timing (right before the release), I would only merge after the release of 0.3 and deep testing.
Question: Would you like me to break this into separate PRs to make it easier for review?
I guess we'll manage that for this time, it'll just take longer.
Question: See // FIXME - modify to check if it is a valid ETC address. Is there a better way to check that the recipient address is an ETC address when sending from an account on the ETC chain? And that recipient address is ETH address when sending from an account on the ETH chain?
Since ETC is a fork, they use the exact same crypto and there is no way to differentiate them
|
|
||
| if (!this.isCurrentChainIdTheAddressForImportChainId(signerChainId)) { | ||
| this.setState({ | ||
| error: `Parity Signer account chainId ${chainIdString} (${ |
There was a problem hiding this comment.
Users don't need to learn yet another term (chain id) or know what chain id Kovan has :)
I'd be more generic and talk about networks:
Network mismatch, please import a Parity Signer account for the XYZ Network.
ps: note that foundation translates to Ethereum mainnet outside of Parity.
There was a problem hiding this comment.
Agreed. Although when the wording becomes longer the error message displays across 3 rows instead of 2 rows, such that the user has to scroll to read it. I assume we'll address this in a the separate PR that's dedicated to notifications #331 ?
There was a problem hiding this comment.
yes, notification will be handled in their own PR for sure.
There was a problem hiding this comment.
I've pushed a commit 1c24567 that addresses this
| status: { good, syncing } | ||
| } | ||
| }) => good || syncing, | ||
| // Only call light.js chainName$ if we're syncing or good |
There was a problem hiding this comment.
why? we don't need to be synced/good to query the chainId RPC
There was a problem hiding this comment.
i just noticed that this approach was used elsewhere in the code when chainName was being used, so i just borrowed that approach thinking it was necessary
There was a problem hiding this comment.
I think it's only used in Health.js, and the reason was we didn't want to make an RPC call before having set the API (since Health.js is always rendered). However, the AccountImportOptions screen can only be accessed through navigation inside the app, so the api will necessarily be set by then.
There was a problem hiding this comment.
I've pushed commit 20e3088 that addresses this comment
| const chainId = parseInt(chainIdString); | ||
| const signerChainId = parseInt(chainIdString); | ||
|
|
||
| if (!this.isCurrentChainIdTheAddressForImportChainId(signerChainId)) { |
There was a problem hiding this comment.
nitpick but the chainId isn't an information inside the address like the function name could suggest, it's more like a meta data that comes alongside with the address
maybe currentChainId !== signerChainId and remove the function? we don't have to use bignumber here afaik
edit: ah, forgot that the api returns a BN. then we can use BigNumber(signerChainId).eq(currentChainId);
| @@ -115,14 +161,20 @@ class AccountImportOptions extends Component { | |||
| }); | |||
| }; | |||
|
|
|||
| hasExistingAddressForImport = (addressForImport, chainId) => { | |||
| isCurrentChainIdTheAddressForImportChainId = chainIdInt => { | |||
There was a problem hiding this comment.
as I wrote before I would remove this function altogether
There was a problem hiding this comment.
i've addressed this issue in commit c4cf7d5
| return BigNumber(chainIdInt).eq(currentChainIdBN); | ||
| }; | ||
|
|
||
| hasExistingAddressForImport = (addressForImport, chainIdInt) => { |
There was a problem hiding this comment.
not related to this PR: it took me a while to understand what the function does, name is a bit confusing. maybe rename to accountAlreadyExists ?
There was a problem hiding this comment.
i've addressed this in commit cab245e
| @@ -378,7 +380,7 @@ class TxForm extends Component { | |||
| * Prevalidate form on user's input. These validations are sync. | |||
| */ | |||
| preValidate = values => { | |||
| const { balance, token } = this.props; | |||
| const { balance, chainId: currentChainIdBN, token } = this.props; | |||
There was a problem hiding this comment.
cc @amaurymartiny the chainId$() rpc returns a bignumber, but AFAIK we could just return a regular int and it might simplify post treatment; no need for bignumber.
what's the consensus on this? are all numbers returned from @parity/api BigNumber? if so we should keep it this way; if not do we want to return regular ints for "simple" numbers? or would it be more confusing?
There was a problem hiding this comment.
I think it could be a regular in too, but all the others RPC calls need to be BigNumber so it would be the only exception (i.e. to check balance, block number, transaction count, peer count, etc), so I agree with you that we should keep it as a BigNumber.
FYI, I had a look around and found that the chainId$ comes @paritytech/ light.js in https://github.com/paritytech/js-libs/blob/master/packages/light.js/src/rpc/eth.ts#L21, where the Typescript indicates returns type BigNumber | Symbol (in that file it looks like other calls also return this type), which calls from rpc/eth in @parity/ api https://github.com/paritytech/js-libs/blob/master/packages/api/src/rpc/eth/eth.js#L42 in light.js.
The API docs show that this type is returned here https://paritytech.github.io/js-libs/light.js/api/modules/_rpc_eth_.html
In the Parity Ethereum Rust codebase under rpc/eth the eth_chainId returns U64 https://github.com/paritytech/parity-ethereum/blob/master/rpc/src/v1/traits/eth.rs#L55
There was a problem hiding this comment.
Sorry, I just saw this.
are all numbers returned from @parity/api BigNumber?
Yes, and I would keep it like that for consistency. Trick: instead of using .valueOf() .toNumber() or anything else, +chainId also works.
| return { to: 'Please enter a valid Ethereum address' }; | ||
| // FIXME - modify to check if it is a valid ETC address |
There was a problem hiding this comment.
we can't deduce to what network (eth/etc) the address corresponds based solely on the address. so logic for address validation for eth/etc is the same. a valid eth address is a valid etc address
| } | ||
| : { | ||
| amount: `You don't have enough ${ | ||
| currentChainIdBN.valueOf() === '61' ? 'ETC' : 'ETH' |
There was a problem hiding this comment.
might be worth creating util functions somewhere
isEthChainId(chainId: number | BigNumber) : boolean (that accepts a number or a BN)
chainIdToString(chainId: number | BigNumber) : string
There was a problem hiding this comment.
great idea! i've addressed it in this commit b31e433
|
|
||
| @observable | ||
| jsonString = null; | ||
| bip39Phrase = null; // 12 to 24-words seed phrase |
There was a problem hiding this comment.
nitpick: since we're modifying the lines, let's correct the grammar while we're at it and write 12 to 24-word seed phrase 😅
|
|
||
| @observable | ||
| name = ''; // Account name | ||
| parityPhrase = null; // 11 or 12-words seed phrase (Parity Signer used to generate a 11 words recovery phrase) |
There was a problem hiding this comment.
nitpick: ditto (not related to this PR) 11 or 12-word seed phrase (Parity Signer used to generate a 11-word recovery phrase)
There was a problem hiding this comment.
or even ... an 11-word ...
There was a problem hiding this comment.
it keeps on getting better
There was a problem hiding this comment.
i've pushed commit 53a7c7e
| @@ -55,11 +56,13 @@ export class CreateAccountStore { | |||
| */ | |||
| @action | |||
| clear = async () => { | |||
There was a problem hiding this comment.
not related to this PR: found a theoretical bug afaik
currently clear is async but called simply as this.clear(); in the other functions of the class, before they changed the properties to their new values. I would have assumed that this.clear() would be pushed on the event loop async stack and be run after the end of the calling function (and provoke a bug), however this is not the case; need to research why..
i.e. completely confused by the output of this:
let a = 3;
let clear = async () => {
console.log('inside clear');
a = 0;
}
let run = async () => {
console.log('inside run');
clear();
console.log('inside run - continuing');
a = 4;
}
run();
console.log('post run');
// output:
// inside run
// inside clear
// inside run - continuing
// post run
// would have expected, because of the way sync/async calls and promises are handled in js:
// post run
// inside run
// inside run - continuing
// inside clearneed to brush up on my js
all the same let's remove async from clear and make it a regular function!
There was a problem hiding this comment.
ok, this explains that: The callback passed to a Promise constructor is always called synchronously, but the callbacks passed into then are always called asynchronously I had assumed promises callback always worked like setTimeout
| this.address = null; | ||
| this.bip39Phrase = null; | ||
| this.isImport = false; |
There was a problem hiding this comment.
I don't think we should reset isImport here, afaik clear() only resets the account info
There was a problem hiding this comment.
i've pushed commit 1c8f7f2
| /** | ||
| * If we do not comment out `await this.clear()` here, then in the next step is | ||
| * `componentDidMount` of `AccountName` component, the value of `createAccountStore.address` | ||
| * and `createAccountStore.signerChainId` will be both undefined, even though we tried |
There was a problem hiding this comment.
maybe try again if this.clear() doesn't reset isimport ?
There was a problem hiding this comment.
it was because I was supplying it as await importFromSigner({ address, signerChainIdInt }); and then receiving it as importFromSigner = async ({ address, signerChainId }) => {.
So I've just renamed it to signerChainId to avoid confusion
|
cool! code LGTM apart from a few grumbles |
…ccount-matching-current-chain
… RPC of light.js on pages accessed through navigation
…mported chain id of address
…ing, isNotErc20TokenAddress
…ccount-matching-current-chain
There was a problem hiding this comment.
Haven't been able to test transactions with ETC as I don't have any. Also note that there is no token support for the classic network, so I would suggest to mention this if talking about ETC support.
Finally, it seems that the light client has troubles to import new blocks once it's at the top of the chain. (It's not related to Fether ofc, just a comment).
All and all I'm quite reluctant to "support" ETC as this has never been part of an issue, hence nobody expressed this needs. Asking @amaurymartiny what he thinks.
There was a problem hiding this comment.
tbh I think 1/ this PR doesn't break any functionality with ETH and testnets 2/ fixes some other bugs and 3/ the ETC introduction is not overly complex, so I'm okay to merge with this. Even though yeah the ETC support could have been done in a later PR, as it's super-low priority.
| // SPDX-License-Identifier: BSD-3-Clause | ||
|
|
||
| const isEtcChainId = currentChainIdBN => { | ||
| return currentChainIdBN.valueOf() === '61'; |
| } else if (!values.to || !isAddress(values.to)) { | ||
| } else if ( | ||
| (!values.to || !isAddress(values.to)) && | ||
| !isEtcChainId(currentChainIdBN) |
There was a problem hiding this comment.
I've changed it in this commit to make it clearer: b5296af
Sorry it was confusing code.... it was actually just so if the user is connected to the classic chain id and they enter an invalid Ethereum address, then it would show a tooltip that reminds them that they need to enter a valid Ethereum address, but it takes the opportunity to highlight to them that the recipient address should be an Ethereum Classic address so they don't lose their funds by sending it to a wrong address.
Perhaps a future PR could include an upfront warning on that page to remind the user to double check that the recipient address that they enter is on the chain as the chain that they're currently using, and potentially even check that for them automatically.
| token && | ||
| token.address && | ||
| token.address !== 'ETH' && | ||
| token.address !== 'ETC', |
There was a problem hiding this comment.
isNotErc20TokenAddress
| @@ -54,7 +72,8 @@ const withTokens = compose( | |||
| return { | |||
| tokensArray, | |||
| tokensArrayWithoutEth: tokensArray.filter( | |||
| ({ address }) => address !== 'ETH' // Ethereum is the only token without address, has 'ETH' instead | |||
| // Ethereum and Ethereum Classic are the only tokens without address, has 'ETH' or 'ETC' instead | |||
| ({ address }) => address !== 'ETH' && address !== 'ETC' | |||
There was a problem hiding this comment.
isNotErc20TokenAddress
There was a problem hiding this comment.
i'ved pushed in commit b94d4a2
| const chainId = parseInt(chainIdString); | ||
| const signerChainId = parseInt(chainIdString); | ||
|
|
||
| if (!BigNumber(signerChainId).eq(currentChainIdBN)) { |
There was a problem hiding this comment.
currentChainIdBN.eq(signerChainId) is easier to read, and you don't even need to parseInt
There was a problem hiding this comment.
i've addressed this in commit 0197108
|
@amaurymartiny I've addressed all your review comments and merged the latest master and fixed merge conflicts, including double checking that all text shown in the UI is wrapped with i18n and keys are consistent in en.json and de.json. |
amaury1093
deleted the
luke-360-only-allow-import-parity-signer-account-matching-current-chain
branch
This PR tries to satisfy the following:
Run "start" script in fether-electron's package.json with
--chain poanetworkorand other chain with a genesis file in paritytech/parity-ethereum/ethcore/res/ethereum.
Then try to import an account created in Parity Signer (should show error message since
Parity Signer only allows users to create accounts on 'foundation', 'classic', 'kovan',
or 'ropsten' networks at the moment, but not the one that Fether is currently running)
Run "start" script in fether-electron's package.json with
--chain classicor--chain foundation. Then try to import an account created for a different chain in Parity Signer from a (should show error message), and then try importing an account that matches the one that Fether is currently running and confirm that it allows you to choose and account name and displays it.After importing 'classic' accounts from Parity Signer when running 'classic' chain, and importing 'foundation' accounts from Parity Signer when running 'foundation', try changing between chains and confirm that on the Accounts page only the accounts associated with the current chain are shown.
Verify that after importing an account from Parity Signer into Fether that it adds it to the array in Local Storage under the Key
localforage/__paritylight::paritySignerAccountswith a value such as[{"address":"0x00d78f7613bbef0f279f7f563b77d88d1190891e","name":"cl4","chainId":61}], which includes values for theaddress,nameandchainIdkeys.Verify that after importing an Ethereum Classic account that on the Account page, the Send Ether page, and the Confirmation pages, it shows the Ethereum Classic logo (not the Ethereum logo) next to the text "Ether", and the ticker symbol should be "ETC" instead of "ETH".
Question: See
// FIXME - modify to check if it is a valid ETC address. Is there a better way to check that the recipient address is an ETC address when sending from an account on the ETC chain? And that recipient address is ETH address when sending from an account on the ETH chain?Question: Would you like me to break this into separate PRs to make it easier for review?