fee.toString instead of fee.toNumber #513
Conversation
Yes and it passes now |
src/utils/operation.ts
Outdated
| @@ -16,7 +16,7 @@ export interface ITransaction { | |||
| opts?: { | |||
| gasLimit?: number | |||
| gasPrice?: number | |||
There was a problem hiding this comment.
should gasPrice b e also a sting ? it might not fit into 53 bit number as well..
@jellegerbrandy does this struct is used by common in some way ?
There was a problem hiding this comment.
Does it related to this PR?
Looks like there is no use of gasPrice in joinAndQuit/plugin.ts
There was a problem hiding this comment.
Seem not related, but neither is the change on the next line :)
In any case, to be consistent with most of the usage in arc.js, we would use a BN, not.a string (and not a number) here.
There was a problem hiding this comment.
The change is necessary if I change to "toString" in plugin.ts.
There was a problem hiding this comment.
gasPrice cannot be a number as it is not fit into 53 bits .
it might be BN though
src/plugins/joinAndQuit/plugin.ts
Outdated
| @@ -154,7 +155,7 @@ export class JoinAndQuit extends ProposalPlugin< | |||
| let opts | |||
| if ((await state).pluginParams.fundingToken === NULL_ADDRESS) { | |||
| // if we have no funding token, we shoudl send the fee as ETH | |||
| opts = { value: options.fee.toNumber()} | |||
| opts = { value: new BigNumber(options.fee.toString()).toHexString() } | |||
There was a problem hiding this comment.
I have nothing against this if this works, but in other places we just pass a a string to the contracts (instead of a BigNumber), so this could just read "options.fee.toString()"
There was a problem hiding this comment.
Both changes are depended, and if I change here to "toString" I must change in operation.ts to "string".
In other places in Alchemy we pass also numbers to the contracts and there is no reason that fee would not be a number also.
There was a problem hiding this comment.
fee must be a String here as ether.js look for the value as String for sure it cannot be Number as it not fits in 53 bits
I think as it is with the update it will cover all cases... both passing BN or String
There was a problem hiding this comment.
I have nothing against this if this works, but in other places we just pass a a string to the contracts (instead of a BigNumber), so this could just read "options.fee.toString()"
though the test pass a BN https://github.com/daostack/arc.js/blob/client-2-0/test/proposal-joinAndQuit.spec.ts#L52
src/utils/operation.ts
Outdated
| @@ -16,7 +16,7 @@ export interface ITransaction { | |||
| opts?: { | |||
| gasLimit?: number | |||
| gasPrice?: number | |||
There was a problem hiding this comment.
Seem not related, but neither is the change on the next line :)
In any case, to be consistent with most of the usage in arc.js, we would use a BN, not.a string (and not a number) here.
|
All I ask is that arc.js be consistent with these data types. |
resolves: #512