-
Notifications
You must be signed in to change notification settings - Fork 41
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// TODO: Pending review what makes sense here. How do gas prices are stablished in xDAI network | ||
100: undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does web3.eth.getGasPrice()
return? Is it a variable number over time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll find out soon enough, I'm making a PR only for the gas, see #1398
@@ -1,7 +1,7 @@ | |||
import BN from 'bn.js' | |||
import { assert } from '@gnosis.pm/dex-js' | |||
import { assert, toBN } from '@gnosis.pm/dex-js' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Importing toBN
from dex-js
is ok here because it ultimately comes from web3
which is a peer dep for dex-js
and therefore local web3
is used. But I would be wary of just importing any utility function because of potential version conflictsor duplicating deps.
For example if we ever need a function that makes use of bignumber.js
and import it by the way of dex-js
, it would be good to move bignumber.js
to peer deps too.
Part of #1398
First of several xDAI PRs. The xDAI integration is not finished, see #1398 for more pending tasks
This one includes:
Depend on gnosis/dex-js#184