Address comments for PR 6

[JakeLin]

In this PR, I addressed the comments for PR #6

@adelRestom I have addressed all your comments except for the App.js.

App.js

1. Very important thing we usually do is to protect the user from losing Ether on function calls that would fail; we do that by first calling the function with .call() which runs it locally/simulation (locally => no transaction => no mining => no gas consumed => no Ether lost), if that call doesn't fail then we proceed to actually call the function (i.e. call it without .call.
2. You used on('transactionHash'), also use on('receipt') to inspect the receipt status (that it's == 1) and to inform the user of a successful transaction

For the first comment, do you call all the send method like
await contract.methods.split(beneficiary1Address, beneficiary2Address).call();
before

await contract.methods.split(beneficiary1Address, beneficiary2Address).send({
  from: accounts[0],
  value: web3.utils.toWei(etherToSplit, 'ether')
});

For the second comment, I have checked the receipt in the promise call - see tx variable, please.

  const tx = await contract.methods.split(beneficiary1Address, beneficiary2Address).send({
        from: accounts[0],
        value: web3.utils.toWei(etherToSplit, 'ether')
      }).on('transactionHash', txHash => {
        this.setState({ splitMessage: `Transaction ${txHash} submitted, waiting for the network to mine` });
      }).on('receipt', receipt => {
        console.log(receipt);
      });
      console.log(tx);

I print out receipt and tx, they are exactly same.

@xavierlepretre can you please have a look as well? thanks

[xavierlepretre]

What Adel meant is:

const result = await contract.methods.split(beneficiary1Address, beneficiary2Address).call(...
if (result) {
    do the .send

[xavierlepretre]

As for the receipt, I see you are checking its status 👍 . You could also check the presence of the expected event.

[JakeLin]

@xavierlepretre @adelRestom

I have tried out the following code

   const result = await contract.methods.split(beneficiary1Address, beneficiary2Address).call({
        from: accounts[0],
        value: web3.utils.toWei(etherToSplit, 'ether')
      });
      console.log(result);
      const tx = await contract.methods.split(beneficiary1Address, beneficiary2Address).send({
        from: accounts[0],
        value: web3.utils.toWei(etherToSplit, 'ether')
      }).on('transactionHash', txHash => {
        this.setState({ splitMessage: `Transaction ${txHash} submitted, waiting for the network to mine` });
      });
      if (tx.status) {
        const contractBalance = web3.utils.fromWei(await web3.eth.getBalance(contractAddress));
        this.setState({ splitMessage: `Transaction ${tx.transactionHash} got mined successfully.`, contractBalance });
      } else {
        this.setState({ splitMessage: `Something went wrong with transaction ${tx.transactionHash}.` });
      }

I found out whether the transaction fails or not in await contract.methods.split(beneficiary1Address, beneficiary2Address).send(... , await contract.methods.split(beneficiary1Address, beneficiary2Address).call(... will always returns an object of Result like

Can you guys tell me how can I meet @adelRestom 's requirement?

[adelrustum]

I think this line of code becomes irrelevant because the normal withdraw is locked now.

[adelrustum]

Make this functionality part of the Pausable contract, we want to protect the user from accidentally calling this function by turning it into a 2 step-process:

1. The user pauses the contract
2. The user kills the contract (the kill function checks that the contract is paused)

[adelrustum]

Add a modifier called whenPaused; you'll need it for "kill" function anyway.

[adelrustum]

You'll also need to update your migration script and test file to pass the value to the constructor when calling new().

[adelrustum]

Regarding the .call() returned object, I thought it would throw an exception for a failed call that you can catch with a .catch().
Try to change your split function so that it returns a boolean and add return true; at the end of the function, this way you can check the returned boolean value so it would be like Xavier's code (let result = contract.func.call; if result == true..then call send).

[JakeLin]

@adelRestom I have followed your advice to add return true in the function like

  function split(address _beneficiary1, address _beneficiary2) external payable whenRunning whenAlive returns(bool) {
    require(_beneficiary1 != address(0) && _beneficiary2 != address(0), "Beneficiary's address must not be zero!");
    require(msg.value > 0, "Must split more than zero ether!");

    if (msg.value.mod(2) == 1) {
      balances[msg.sender] = balances[msg.sender].add(1);
    }
    uint256 half = msg.value.div(2);
    balances[_beneficiary1] = balances[_beneficiary1].add(half);
    balances[_beneficiary2] = balances[_beneficiary2].add(half);
    emit LogSplitted(msg.sender, msg.value, _beneficiary1, _beneficiary2);
    return true;
  }

And add the call() check in App.js like

      const result = await contract.methods.split(beneficiary1Address, beneficiary2Address).call({
        from: accounts[0],
        value: web3.utils.toWei(etherToSplit, 'ether')
      });
      if (!result) {
        return;
      }

However, even I send 0 wei to the contract, result is still true. I also tried the same approach in withdraw function. They always return true when I call call().

So I don't know how can I check before I send().

[adelrustum]

@xavierlepretre can you please remind me how we used to do this? ☝️

[xavierlepretre]

I cloned your repo, checked out this branch, replaced 9545, npm installed both folders, truffle migrated into Ganache, npm started.
When I open in the browser, I get:

index.js:1375 TypeError: contract.methods.owner is not a function
    at App.componentDidMount (App.js:64)
console.<computed> @ index.js:1375

[xavierlepretre]

I would try to console.log(typeof result) and console.log(JSON.stringify(result)) to collect more info.

[JakeLin]

@xavierlepretre I just merged the PR, can you rebase master?

[adelrustum]

I'm looking at my old work and I had something like:

return myContract.myFunction.call()
.then(() => myContract.myFunction())
.then...more code
.catch(error => console.log(error));

So the catch would show the error.

[JakeLin]

thanks @adelRestom I am working on project 2 and 3 to try to make meet the deadline, will come back to this soon.

[xavierlepretre]

I still have the same error when loading the web page:

index.js:1375 TypeError: contract.methods.owner is not a function
    at App.componentDidMount (App.js:64)
console.<computed> @ index.js:1375

[JakeLin]

@xavierlepretre sorry, for some reason, I didn't commit the fix to github. I have pushed the commit 522d3d8 now, it should be OK now.

[xavierlepretre]

Now I get a revert on withdraw. "Balance must be grater than zero to withdraw!"
Even though it tells me that the account I split to has 0.5 Ether.

[JakeLin]

That's weird, may I come back to this later? I am trying to finish the other projects