[NayNay] Signing bug for validators

[rh0delta]

• updated signing with adapters to retry the signing request if a specific error is hit as defined in this issue [bug] with more then two validators signing is sometimes broken sdk#367

closes entropyxyz/sdk#367
close #77

[mixmix]

🔥 this is a global variable. The way you're using it means that this function will try fixing once, then it will not be reset to 0 after this signing... so all future buggy signings will be skipped.

This function feels like it's way too fancy, and also not really named what it is. I *\think* all that we need is for the original code to be the same but like this:

const sigOpts = {
  msg: msgParam,
  order: ['deviceKeyProxy', 'noop'],
}

const signature = await entropy.signWithAdaptersInOrder(sigOpt)
  .catch(err => {
    // HACK: sometimes order is reversed D:
    sigOpts.order.reverse()
    return entropy.signWithAdaptersInOrder(sigOpt)
  })

[mixmix]

NOTE this code assumes signWithAdaptersInOrder DOES NOT MUTATE the input... that would need checking.
We should not write functions which mutate inputs though 💀

[mixmix]

don't see any mutations, should be fine?

[rh0delta]

🔥 this is a global variable. The way you're using it means that this function will try fixing once, then it will not be reset to 0 after this signing... so all future buggy signings will be skipped.

This function feels like it's way too fancy, and also not really named what it is. I *\think* all that we need is for the original code to be the same but like this:

const sigOpts = {

  msg: msgParam,

  order: ['deviceKeyProxy', 'noop'],

}



const signature = await entropy.signWithAdaptersInOrder(sigOpt)

  .catch(err => {

    // HACK: sometimes order is reversed D:

    sigOpts.order.reverse()

    return entropy.signWithAdaptersInOrder(sigOpt)

  })

We could do this way of catching the error, but we still need to keep track of the attempts made to signing and handle accordingly. I find the try..catch to be cleaner, thoughts?

[mixmix]

• try / catch is the same as promise.catch 🤷 (don't mind)
• your global variable is still a liability - if multiple people are calling this function your global variable becomes a race condition

You need to change this to not use it - either the way I suggested, or pass the attempts count as an argument

async function signWithAdaptersInOrder (entropy, signingData, attempts = 0) {
  if (attempts > 1) throw Error('NOPE')
  // ...

  const newSignData = { /* ... */ } // shuffle
  return signWithAdaptersInOrder(entropy, newSignData, attempts + 1)
}

[rh0delta]

@mixmix ready for re-review

[mixmix]

@rh0delta that 🔥 is still an issue. I've added more context why it's dangerous
https://github.com/entropyxyz/cli/pull/63/files#r1628548871

[rh0delta]

@mixmix this is ready for re-review. I followed your idea of passing in the signing attempts, but rather than having the check for the number of attempts at the top, I've added it to the conditional for retrying the signing request with the shuffled order