-
Notifications
You must be signed in to change notification settings - Fork 1
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
Start off-ramping from evm networks #323
Start off-ramping from evm networks #323
Conversation
✅ Deploy Preview for pendulum-pay ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
src/hooks/useInputTokenBalance.ts
Outdated
|
||
// TODO maybe improve: if the user switches the network in the selector and REJECTS the switch in wallet, then balance will be 0. |
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.
Metamask needs this explicit acceptance, while Talisman for instance, does not.
Since useReadContract
uses the chain that is connected in the wallet, it will read from an incorrect chain.
If this is a UI requirement, we could fetch with the correct chain using the selectedNetwork
parameter.
…t-off-ramping-from-evm-networks
const outputTokenDecimals = OUTPUT_TOKEN_CONFIG[outputTokenType].decimals; | ||
|
||
const inputAmountBig = Big(amountIn); | ||
const inputAmountRaw = multiplyByPowerOfTen(inputAmountBig, inputTokenDecimals || 0).toFixed(); | ||
const pendulumAmountRaw = multiplyByPowerOfTen(inputAmountBig, pendulumDecimals || 0).toFixed(); |
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.
We were previously using inputAmountRaw
for all operations on Pendulum after writing the off-ramping state. Since the decimals on the starting chain and axlUSDC
on Pendulum where the same (6
), there was no issue.
BSC uses 18 decimals so we need to account for this, or potentially for a different token combination.
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.
In a separate PR we can refactor it into a zustand store
@@ -1,10 +1,11 @@ | |||
import { polygon } from '@reown/appkit/networks'; | |||
import { polygon, bsc, arbitrum, base, avalanche, mainnet } from '@reown/appkit/networks'; |
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.
Maybe we should import it once. Now we import it here and in networks.ts
. This may cause problems in the future if we forget to import it in one of these two places
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.
Could be. But this are just constants right? Or you mean, to have a centralized place to handle network information in case we deviate from appkit's definitions.
@pendulum-chain/devs I have refactored the code, please review. Changes:
|
if (network !== polygon.name) { | ||
throw new Error(`Network ${network} is not supported`); | ||
let compatibleNetwork = network; | ||
if (!isNetworkEVM(network)) { |
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'm not so sure we want to show the fee comparison of Polygon when on AssetHub. If we throw here, then it will display N/A
which in my opinion is better. Especially since AssetHub has some higher costs.
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.
For AssetHub we want to show Polygon prices.
<div
className="tooltip tooltip-primary before:whitespace-pre-wrap before:content-[attr(data-tip)]"
data-tip="Quotes are for Polygon, as the providers don't support Asset hub."
>
(Polygon)
</div>
Thanks for the improvements @Sharqiewicz . Seems cleaner! I just tested an off-ramp from BSC and on friday, I tested from AssetHub. I think it is good to go. We should only decide what to do with this, to default to one network or show nothing. |
Note: Review and merge after this refactor is merged into staging.
Issue: #292.
Main changes
evm
chains.Extra (optional) changes
zustand
of the hooks forsep24
.Tests
Off-ramps tested to
ARS
: