gasLimit is currently hard-coded on constants.
It should be a parameter that could be overridden and takes default value from constants.

Transactions from the pool are being removed only when a 404 response is returned from the API. Sometimes, they will be forged but also being returned from the API, so we need to remove them from the pool also when the state is fged.

Currently we are using U2F protocol for communication with the HW Wallets but we should use HID for Node.

Issue

• EIP712 is not yet supported by hardware wallets

Solution

• Retrieve hash message from EIP712 data type and sign in with signer.signMessage(message)

Info collected

• ethers abstract class for private key: https://github.com/ethers-io/ethers.js/blob/ce8f1e4015c0f27bf178238770b1325136e3351a/packages/wallet/src.ts/index.ts
• ethers abstarct class for JSONRpcProvider: https://github.com/ethers-io/ethers.js/blob/ce8f1e4015c0f27bf178238770b1325136e3351a/packages/providers/src.ts/json-rpc-provider.ts
• ethers encode EIP712 class: https://github.com/ethers-io/ethers.js/blob/ce8f1e4015c0f27bf178238770b1325136e3351a/packages/hash/src.ts/typed-data.ts
• more useful links:
  • example EIP712 ethereum: https://github.com/ethereum/EIPs/blob/master/assets/eip-712/Example.js
  • another example: https://codingturtles.com/?p=1562
• raw eip712 has for signer loaded with private keys could be found here: https://github.com/hermeznetwork/commonjs/blob/feature/raw-eip712/src/tx-utils.js#L468

• Separate tests which requires a running hermez environment (contracts, hermez-node, ...) in order to be ble to run unit testing from github actions
• Setup github actions to:
  • run unit tests on every push & pull on master and develop branches
  • run check linter on every push & pull on master and develop branches

Right now, to make a L2Tx, the steps are the following:

• Call generateL2Transaction to get the tansaction and encodedTransaction data
• Sign the transaction data
• Send L2Transaction

  // generate L2 transaction
  const l2Tx = {
    type: 'Transfer',
    from: srcAccount.accountIndex,
    to: dstAccount.accountIndex,
    amount: hermez.Float16.float2Fix(hermez.Float16.floorFix2Float(amountXfer)),
    fee
  }

  const xferTx = await hermez.TxUtils.generateL2Transaction(l2Tx,
    hermezWallet.publicKeyCompressedHex,
    srcAccount.token)
  hermezWallet.signTransaction(xferTx.transaction, xferTx.encodedTransaction)
  const XferResult = await hermez.Tx.sendL2Transaction(xferTx.transaction, hermezWallet.publicKeyCompressedHex)

I think we should compact this into something more user friendly. If intermediate data is required, it can be returned

  // generate L2 transaction
  const l2Tx = {
    type: 'Transfer',
    from: srcAccount.accountIndex,
    to: dstAccount.accountIndex,
    amount: hermez.Float16.float2Fix(hermez.Float16.floorFix2Float(amountXfer)),
    fee
  }
  const XferResult = await hermez.sendL2Transaction(l2Tx, hermezWallet.publicKeyCompressedHex)

In this functionality https://github.com/hermeznetwork/hermezjs/blob/main/src/tokens.js#L25, I think if allowance is larger than the amount that wants to be approved, no action should be triggered.
Current implementation does an approval of 0 tokens.

An approach could be to perform approve only if allowance.lt(amount) otherwise return null

Improve how we handle POST API requests to multiple coordinators

Hermez nodes sync transactions in the pool between them with Hermez Node v1.7.0. Now it's enough with sending the transaction to a single coordinator (Boot Coordinator) and it will be synced between nodes automatically.

If a user wants to get the BJJ associated to his account from the wallet to share it with a third party, it needs to do Wallet->My Account -> view in batch explorer.
I would like the user to be able to retrieve this BJJ without the explorer.

I would like to run some audit on the code to check if it is resistant against incorrect inputs. I think i have seen some vulnerabilities that incorrect inputs may make the program crash

Current implementation does not allow create an HermezWallet from ethereum address in method createWalletFromEtherAccount.
In case HermezWallet is derived from signer type Wallet, meaning only privateKey is needed, there is no dependency on provider url.
I suggest to slightly modify the code and allow hermezWallet creation from ethereum private key without the need setting a provider.

Let me know your thoughts on this

Right now the hermez-node only supports ChainID = 0 for checking signatures.
hermezjs should be updated to hardocde this CHainID until hermez-node is ready

I have s coordinator that has won the next slots. I send a transfer, and I see that the new coordinator never forges this transaction. However, I check the transactions-pool and this transaction is not present in the new coordinator, but it is present in the boot coordinator transactions-pool.

Right now the API returns the array of siblings set to 0 instead of not being present. This will be fixed eventually but until then hermezjs should filter the 0ed siblings.

Add support for withdrawCircuit functionaility

Investigate fee index computation since it returns unexpected values

Currently breaking as it takes longer to forge batches than planned

Summary of Bug

On request for tokens with filtering by multiple (more than one) tokenIds or tokenSymbols returns empty list.

Steps to Reproduce

hermez.CoordinatorAPI.getTokens(null, ['ETH', 'HEZ']);

Additional Information & Screenshots:

In getTokens function tokenIds & tokenSymbols are join with coma ,, but according to API they should be join with pipe |.

We need to add hardware wallets tests

git clone https://github.com/hermeznetwork/integration-testing.git
Looks like it's not public?

I got failure: (node:13751) UnhandledPromiseRejectionWarning: Error: connect ECONNREFUSED 127.0.0.1:8086
I suppose I should clone integration-testing.git and deploy a local setup ?

The following is a list of changes suggested in #22 and not applied due to some time constraints. I leave them here for future consideration:

• src/api.js

  return extractJSON(axios.get(`${baseApiUrl}/accounts`, { params }))

  try {
    const retVal = await axios.get(`${baseApiUrl}/accounts`, { params })
 
@AlbertoElias AlbertoElias 2 hours ago 
We should remove this try catch as the try catch should be done when executing the api call by the developer

• src/contracts.js

 * @return {Contract} The request contract
 */
function getContract (contractAddress, abi, providerUrl) {
function getContract (contractAddress, abi, providerUrl, ethereumAddress) {
  // TODO cache not valid if using different EtherAddress Disable for now

AlbertoElias 2 hours ago 
What do you mean?

• src/tx-utils.js

 * @param {object} transaction - Transaction object sent to generateL2Transaction
 * @return {string} transactionType
 */
function getTransactionType (transaction) {
 
@AlbertoElias AlbertoElias 2 hours ago 
Why remove this?

    nonce++
  let nonce = currentNonce

  if (typeof nonce === 'undefined') {
 
@AlbertoElias AlbertoElias 2 hours ago 
I don't get this. Why would it be undefined? The idea would be for developers to provide whatever the current nonce is. If you think it's better to retrieve it all automatically ourselves, we should just remove the currentNonce parameter

• src/tx.js

import HermezABI from './abis/HermezABI.js'
import WithdrawalDelayerABI from './abis/WithdrawalDelayerABI.js'

/**
 * Get current average gas price from the last ethereum blocks and multiply it
 * @param {number} multiplier - multiply the average gas price by this parameter
 * @param {string} providerUrl - Network url (i.e, http://localhost:8545). Optional
 
@AlbertoElias AlbertoElias 2 hours ago 
Why was this removed?

  const ethereumAddress = getEthereumAddress(hezEthereumAddress)
  let account = (await getAccounts(ethereumAddress, [token.id])).accounts[0]
  const hermezContract = getContract(contractAddresses.Hermez, HermezABI, providerUrl, ethereumAddress)
  let account = await getAccounts(hezEthereumAddress, [token.id])
 
@AlbertoElias AlbertoElias 2 hours ago 
I would rewrite as:

const accounts = await getAccounts(hezEthereumAddress, [token.id])
const account = typeof accounts !== 'undefined' ? accounts.accounts[0] : null
 
@AlbertoElias AlbertoElias 2 hours ago 
We should avoid using let as much as possible, and we shouldn't call account when the value is the result of getAccounts

@@ -82,11 +88,14 @@ const deposit = async (amount, hezEthereumAddress, token, babyJubJub, providerUr
 * @returns {promise} transaction parameters
 */
const forceExit = async (amount, accountIndex, token, gasLimit = GAS_LIMIT, gasMultiplier = GAS_MULTIPLIER) => {
  const hermezContract = getContract(contractAddresses.Hermez, HermezABI)
  // TODO. Check call below as it can be invalid if accountIndex doesn't exist
  const hermezEthereumAddress = (await getAccount(accountIndex)).hezEthereumAddress
 
@AlbertoElias AlbertoElias 2 hours ago 
I would repeat the suggested structure above

• test/tx.test.mjs

jest.mock('axios')

// Skipping this test. We cover transaction flow in hermez-sandbox.test.mjs
 
@AlbertoElias AlbertoElias 2 hours ago 
If you're skipping, can you leave things how they were so that they work with Swagger?

New features

maxNumBatch

Allow the user to sign a maxNumBatch parameter when sending a L2 transaction.

• Proposals implementation:
  • Add new param to transaction maxNumBatch which would be chosen by the user
  • Add numBatchOffset and internally compute the maxNumBatch

Atomic transactions

All the rq should be added to allow the user to link transactions.

• Proposals implementation:
  • when a L2 transaction is done, return the necessary data which could be shared afterwards to link transactions. Therefore allow a single object as a parameter to link transactions
  • Add single parameters just related to atomic transactions and sign them internally