-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: when exact out quote swaping gets bigInt error #334
Fix: when exact out quote swaping gets bigInt error #334
Conversation
- improve tokens names - improve test spec
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.
Improvements look good, left a few comments to improve a bit more.
const routerContract = getRouterContract() | ||
|
||
// Swapr Algebra Contract fee param is uint24 type | ||
const algebraFee = parseFloat(this.fee.multiply(JSBI.BigInt(1_000_000)).toFixed(1)) |
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.
Let's turn this into a const, like
const ALGEBRA_FEE_PARTS_PER_MILLION = JSBI.BigInt(1_000_000)
or just PARTS_PER_MILLION
or MILLION
or _1_000_000
or _1000000
and use it here.
We have similar constants inside /constants folder, like _10000
. Maybe we can create a new one there.
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.
Also, let's simplify this logic. I read .toSignificant()
and .toFixed()
code and we can use this:
const algebraFee = parseFloat(this.fee.multiply(JSBI.BigInt(1_000_000)).toFixed(1)) | |
const algebraFee = this.fee.multiply(ALGEBRA_FEE_PARTS_PER_MILLION).toSignificant(1) |
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 can turn it into a const, but I'll rather have it here next to where it's used.
) | ||
|
||
const routes = await getRoutes(tokenIn, tokenOut, chainId) | ||
const routes = await getRoutes(setToken, quoteToken, chainId) | ||
|
||
const fee = | ||
routes?.length > 0 && routes[0].pools.length > 0 | ||
? new Percent(routes[0].pools[0].fee.toString(), '1000000') |
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.
Let's also use ALGEBRA_FEE_PARTS_PER_MILLION
here, instead of '1000000'
.
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.
const ALGEBRA_FEE_PARTS_PER_MILLION = JSBI.BigInt(1_000_000)
this is different then "1000000"
2c81617
to
128a29d
Compare
|
||
// Swapr Algebra Contract fee param is uint24 type | ||
const algebraFee = this.fee.multiply(ALGEBRA_FEE_PARTS_PER_MILLION).toSignificant(1) | ||
console.log('algebraFee:', algebraFee) |
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.
Undesired console.log('algebraFee:', algebraFee)
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.
Looks good after removing console.log
128a29d
to
61d55a3
Compare
* improve error message * fee param on SwaprV3 needs to be uint24 type so it needed conversion. - improve tokens names - improve test spec * small improvements
* improve error message * fee param on SwaprV3 needs to be uint24 type so it needed conversion. - improve tokens names - improve test spec * small improvements
fee param on SwaprV3 needs to be uint24 type so it needed conversion.