(Fix) Contract interaction ABI resets#1394
Conversation
fernandomg
requested review from
Agupane,
dasanra,
francovenica,
matextrem and
nicosampler
|
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
|
Travis automatic deployment: |
|
Travis automatic deployment: |
|
|
||
| React.useEffect(() => { | ||
| const validateAndSetAbi = async () => { | ||
| if (!mustBeEthereumAddress(contractAddress) && !(await mustBeEthereumContractAddress(contractAddress))) { |
There was a problem hiding this comment.
I'm the only one having real troubles with this naming convention?
Each time I find this mustBe... with the negation I have to think twice that double negation in semantic is necessary to understand this.
There was a problem hiding this comment.
Yes, you're not alone.
But, for the record, they are inherited from final-form validator definitions.
You can see in the documentation itself https://final-form.org/docs/react-final-form/examples/field-level-validation
If the function returns undefined, it's a valid value, if not it should return a string with the error message.
Maybe we should wrap those in another function that returns a boolean if we're going to use those validators outside the form. WDYT?
There was a problem hiding this comment.
yes Fer, I'm with you on that wrapper. Should be handled this in this PR? or create a new one and replace all the occurrences for these cases?
There was a problem hiding this comment.
At least I would consider adding a conversion like this before the if clause. But if we use them in many places this for sure needs a wrapper
// Undefined is returned when no error found
const isEthereumAddress = mustBeEthereumAddress(contractAddress) === undefined
const isEthereumContractAddress = mustBeEthereumContractAddress(contractAddress) === undefined
|
|
||
| // this check may help in scenarios where the user first pastes the ABI, | ||
| // and then sets a Proxy contract that has no useful methods | ||
| if (hasUsefulMethods(abi) === undefined) { |
There was a problem hiding this comment.
hasUsefulMethods should return a boolean no be consistent, otherwise, it should be getUsefulMethods.
There was a problem hiding this comment.
yes, totally... but this is a validator function. If it returns a string, there's an error with the argument, if the returned value is undefined then the argument was valid.
Once again, the same discussion as above.
| const source = getConfiguredSource() | ||
| const abi = await source.getContractABI(contractAddress, network) | ||
|
|
||
| // this check may help in scenarios where the user first pastes the ABI, |
There was a problem hiding this comment.
should we notify the users of this functionality in the UI? Perhaps if we detect that the ABI it's a proxy, we can show a label saying that he can replace the address and the ABI won't be changed.
There was a problem hiding this comment.
not sure about this, I have no certainty that all the proxy contracts won't have a callable method
|
Travis automatic deployment: |
|
The issue of the ABI field cleaning itself was fixed. I tried with the ABI an address that Richard gave in the issue and it works fine in this PR Test:
Looks good to me |
|
Travis automatic deployment: |
6 participants
This PR closes #1328, by replacing the
decoratorthat requests the ABI information with a fieldmutatortriggered withinContractABIcomponent's lifecycle.For some reason (form constantly re-rendered?), the decorator lost the status between renders. So I wasn't able to evaluate current vs previous state, to take the decisions that I'm now able to take in the
ContractABI'suseEffect.This PR also comes with a plus: the user can paste the MasterCopy address of a Proxy-based contract, load the ABI and then replace the address with the Proxy address, and the ABI won't be replaced (if the Proxy has no read/write methods).