search

omgnetwork / omg-js

JavaScript Library for communication with OMG network
Apache License 2.0
42 stars 15 forks source link

createTransaction() will fail if the payment amount is a string #321

Closed kevsul closed 3 years ago

kevsul commented 4 years ago

Childchain.createTransaction() allows you to pass payment.amount as a string or a number, but it will fail in the Watcher if it's a string.

E.g. this fails

{ 
  owner: '0x0',
  payments: [ { owner: '0x0', currency: '0x0', amount: '300000'} ],
  fee: { currency: '0x0' }
}

but this succeeds

{ 
  owner: '0x0',
  payments: [ { owner: '0x0', currency: '0x0', amount: 300000} ],
  fee: { currency: '0x0' }
}

The solution is to either

  1. Only accept number (or BigNumber) as types for payment.amount
  2. Keep accepting String, but convert it to number before calling the watcher api
nicholasmueller commented 4 years ago

leaning towards option 1. also since passing an unsafe integer doesn't give the most descriptive error message when integer validation fails.

should also test this behavior on childchain.sendTransaction as thats the other one that accepts the payments object.

we may also want to only accept BN across our entire api to reduce confusion of these multiple types, ie the rootchain module and transaction helpers

Pongch commented 4 years ago

I'm supportive of 1, it's easier to document.

However, if we do decide to stick with 2, perhaps something like property based testing could protect us from issues like this one across our APIs in the future?