pending changes from code review

[druiz0992]

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?