Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Luizoamorim/from ilyas browser fixes 2 #670

Merged
merged 21 commits into from
May 27, 2022

Conversation

luizoamorim
Copy link
Contributor

@luizoamorim luizoamorim commented May 18, 2022

In this PR I'll be fixing some issues from here : https://github.com/EYBlockchain/nightfall_3_private/issues/74 for browser.

Browser issues

  • Issue 1 - If you put an ethereum address instead of a pkd L2 address then you get some error in the console and the modal of generating the proof stays forever

Do a verification using the difference of length between l1 hash and l2 hash.

  • Issue 4 - L2 Bridge page - transact MAX - Doesn't work - console error: Uncaught TypeError: Cannot set properties of null (setting 'value')

  • Issue 5 - Deposits - can create and send transactions for amounts bigger than my current balance. Can create the commitment successfully, but when sending the tx Metamask will console.log an RPC error - the user won't notice (image attached in comments below). You end up with an 'invalid' commitment.

I think the solution is block this kind of transactions in frontend.

  • Issue 6 - Deposits - can create and send transactions with no value, however the process seems to 'complete' not causing side effects - hence the low priority

What I saw:

  • For send transaction is not being verified the address neither the ammount

  • For withdraw if you input a ammount greater than your balance the modal is opening

  • For deposit the modal is not opening for 0 ammount or ammount greater than balance, but is not showing any warning.

  • Issue 7 - E.g. I deposit 1 MATIC, after completing the tx, form is still 'set' to 1 MATIC. Suggestion - re-route users to Transactions page if tx is successful.

  • Issue 9 - Restrictions in deposits now it shows a message in red under the amount to warn the user but the Transfer button is not disabled, so I can continue with the deposit of a restricted amount.

  • Issue 10 - In transactions, clicking on arrow takes you to goerli etherscan. In mainnet wallet, this should be mainnet etherscan

@luizoamorim
Copy link
Contributor Author

luizoamorim commented May 18, 2022

Issue 10

I've noticed that the tx in database was not with the L1 transaction hash:

So I added this information for the tx in the submitTx function:

case 'onchain': {
          const txL1Hash = await submitTransaction(
            readyTx.rawTransaction,
            shieldContractAddress,
            150000,
            0,
          ); // 150k is enough gasLimit for a deposit
          readyTx.transaction.l1Hash = txL1Hash;
          break;
}

Other change was make the url for etherscan flexible in Transaction component. For it I did the following verification:

const [etherscan] = React.useState(
    ChainIdMapping[process.env.REACT_APP_MODE].chainName === 'Ethereum Mainnet'
      ? 'etherscan.io'
      : 'goerli.etherscan.io',
  );

And then I changed the url in the arrow link in transaction row:

<a
                      href={`https://${etherscan}/tx/${tx.l1Hash}`}
                      className="etherscanLink"
                      rel="noopener noreferrer"
                      target="_blank"
                      style={{ marginLeft: '20px' }}
                      onClick={e => e.stopPropagation()}
>

You can see this changes in: Commits on May 17, 2022

@luizoamorim
Copy link
Contributor Author

luizoamorim commented May 18, 2022

Issue 4

The commit e3eabc9 is fixing the issue number 4. This way the button MAX back to work.

@luizoamorim
Copy link
Contributor Author

luizoamorim commented May 18, 2022

Issue 5 and part of 6

The commit 40688d1 is fixing the problems for the deposit and withdraws transfers verification.

@druiz0992 druiz0992 requested a review from IlyasRidhuan May 19, 2022 06:10
@luizoamorim
Copy link
Contributor Author

Issue 1 and the rest of 6

The last commits have the changes to fix the problems in send modal:

  • Verification if the address is in correct format.
  • Verification if the user put a balance greater than zero or nor greater than its balance.

@luizoamorim
Copy link
Contributor Author

Issue 9

In commit 40688d1 line 329 I added the restriction for deposit.
I did put 0.25 hardcoded, maybe we can think in have it like some env var.

line 329:

if (txType === 'deposit' &&
        new BigFloat(transferValue, token.decimals).toBigInt() > new BigFloat('0.25 ', token.decimals).toBigInt()
    ) {
      toast.error("Input value can't be greater than 0.25");
      return;
}

@luizoamorim
Copy link
Contributor Author

Issue 7

Was added a new feat in the last commits. If the subimitTx function return true, the user is redirected for transactions page.

Base automatically changed from ilyas/browser-fixes-2 to master May 24, 2022 11:17
@luizoamorim
Copy link
Contributor Author

The last commits are solving the hardcoded issue @IlyasRidhuan

@IlyasRidhuan IlyasRidhuan merged commit 6e56602 into master May 27, 2022
@IlyasRidhuan IlyasRidhuan deleted the luizoamorim/from-ilyas-browser-fixes-2 branch May 27, 2022 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants