-
Notifications
You must be signed in to change notification settings - Fork 0
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
add PEN and XLM tokens to Astar portal #1
add PEN and XLM tokens to Astar portal #1
Conversation
Tested PEN using Cross-chain Transfer(XCM)Transferred to Astar:Sent back PEN to Pendulum:Here are the extrinsic transactions: Test for minimum 0.1 PEN: |
src/modules/xcm/index.ts
Outdated
[Chain.PENDULUM]: { | ||
name: Chain.PENDULUM, | ||
relayChain: Chain.POLKADOT, | ||
img: require('/src/assets/img/token/pen.png'), |
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.
How about we add the image as an SVG instead so that it's easily scalable.
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 didn't see a PEN.svg in our drive...?
https://drive.google.com/drive/u/0/folders/1sZcQzoDxT_EUd2wtEJN8ECNzPakfAMs6 only PNG.
I'll ask Anna in the ticket to provide us one.
oh, nvm. the image was
and I missed it.
ok.
src/modules/xcm/tokens/index.ts
Outdated
symbol: 'PEN', | ||
isNativeToken: true, | ||
assetId: '18446744073709551634', | ||
originAssetId: '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.
Looking at the other definitions, I think this should rather be
originAssetId: '0', | |
originAssetId: 'PEN', |
though I'm not 100% sure if this is important.
|
||
try { | ||
// PEN | ||
if (token.originAssetId == '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.
We would need to change this to == 'PEN'
then probably
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.
Besides that, we do get a isNativeToken
boolean passed into this function already. What does that do?
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.
updated
if (token.id !== '18446744073709551634') { | ||
throw 'Token must be PEN'; | ||
} |
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 this condition is too strict and we should remove it. Otherwise, we'll have to change it again once we support more tokens other than PEN.
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've updated to add XLM and EURC
if (token.id !== '18446744073709551634') { | ||
throw 'Token must be PEN'; | ||
} | ||
let tokenData = PEN; |
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.
Then we would probably need to change this to let tokenData = token.originAssetId === "PEN" ? "Native" : { XCM: token.originAssetId }
or similiar. Not sure what exactly token.originAssetId
can be.
Tested XLM using Cross-chain Transfer(XCM)Here are the extrinsic transactions using 0.1 XLM: |
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 to me without the console log.
Let's close this PR then and create one to the upstream repo @b-yap
Pull Request Summary
Add support for PEN asset
Closes https://github.com/pendulum-chain/tasks/issues/310 and https://github.com/pendulum-chain/tasks/issues/314
This pull request makes the following changes:
Adds
Fixes
Changes