(Bugfix) Fails to create transaction

[fernandomg]

This PR fixes an issue in development where transactions are not being properly created due to nonce not being properly calculated.

[github-actions]

CLA Assistant Lite All Contributors have signed the CLA.

[github-actions]

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

• Result: ✅ success
• Annotations: 0 total

---

^{Report generated by eslint-plus-action}

[ghost]

Travis automatic deployment:
https://pr1332--safereact.review.gnosisdev.com/app

[mmv08]

@fernandomg Agustine made a solution for this in his PR #1230, maybe you can take it from here to avoid conflicts in the future

[mmv08]

On the other hand, why did it stop working at development? This function has been working fine for ages. May it be that we're dealing with the consequences instead of solving the real problem?

[rmeissner]

Please add a test for this to make sure that this doesn't happen again

[fernandomg]

@mikheevm,

Agustine made a solution for this in his PR #1230, maybe you can take it from here to avoid conflicts in the future

I didn't know he was working on a particular bug that affects all the other PRs, in that particular one. Good to know.

On the other hand, why did it stop working at development? This function has been working fine for ages. May it be that we're dealing with the consequences instead of solving the real problem?

That's the exact same question I asked myself. The bug was introduced here: https://github.com/gnosis/safe-react/pull/1301/files#diff-6c3bab67fc37510237bb32f6523ef7f9R19-R32

If you see, I think that this fix is more likely to keep the function behavior from before. The as string part was unnecessary and forced an invalid type, thus, the returned value was wrong.

I just added the !txNonce because if it's undefined or '' will ask the safeInstance for the nonce.

@rmeissner,

Please add a test for this to make sure that this doesn't happen again

Agree, but I'm still waiting for the #1230 to be merged to have the mocked services that help to test.

[mmv08]

The as string part was unnecessary and forced an invalid type

The return type for the function is string, how could it force an invalid type?

[fernandomg]

The as string part was unnecessary and forced an invalid type

The return type for the function is string, how could it force an invalid type?

you're returning undefined as string.

[Agupane]

I do have tests for this on my test branch, don't lose time adding them here

[mmv08]

@fernandomg function's return type is Promise<string>, in this case, it needs to be adjusted

[fernandomg]

@fernandomg function's return type is Promise<string>, in this case, it needs to be adjusted

That comment isn't adding any significant value to the discussion. Being a Promise or not, does not change the fact that txNonce as string is just wrong.

Thing is that the function "worked well for ages", and a bug was introduced due to enforced types.

Check the old code:

export const getNewTxNonce = async (txNonce, lastTx, safeInstance) => {
  if (!Number.isInteger(Number.parseInt(txNonce, 10))) {
    return lastTx === null
      ? // use current's safe nonce as fallback
        (await safeInstance.nonce()).toString()
      : `${lastTx.nonce + 1}`
  }
  return txNonce
}

the bug was introduced here:

export const getNewTxNonce = async (
  txNonce: string | undefined,
  lastTx: TxServiceModel | null,
  safeInstance: GnosisSafe,
): Promise<string> => {
  if (typeof txNonce === 'string' && !Number.isInteger(Number.parseInt(txNonce, 10))) {
    return lastTx === null
      ? // use current's safe nonce as fallback
        (await safeInstance.methods.nonce().call()).toString()
      : `${lastTx.nonce + 1}`
  }

  return txNonce as string
}

the solution I'm proposing is: if txNonce is falsy (that is undefined or ''), then enter the if, if not, return the txNonce.

We can't keep the original code as Number.isInteger doesn't accept undefined as a argument.

to avoid confusions, what I'm proposing in this PR is:

export const getNewTxNonce = async (
  txNonce: string | undefined,
  lastTx: TxServiceModel | null,
  safeInstance: GnosisSafe,
): Promise<string> => {
  if (!txNonce || !Number.isInteger(Number.parseInt(txNonce, 10))) {
    return lastTx === null
      ? // use current's safe nonce as fallback
        (await safeInstance.methods.nonce().call()).toString()
      : `${lastTx.nonce + 1}`
  }

  return txNonce
}

[mmv08]

@fernandomg sorry, I mixed it up a little bit. But nevertheless, I like Agustine's solution more because it's easier to understand.

[fernandomg]

@mikheevm, updated with Agus' code.

@rmeissner, can you approve this? See: #1332 (comment)

This bug is blocking those PRs who already merged development

Except that if #1230 is getting merged right away, this PR makes no sense after all and we can close it.

[ghost]

Travis automatic deployment:
https://pr1332--safereact.review.gnosisdev.com/app

[fernandomg]

closed, as it's no longer needed due to the revert at: #1335