-
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
Fix for incorrect assetCode property #280
Fix for incorrect assetCode property #280
Conversation
✅ Deploy Preview for pendulum-pay ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Isn't it almost more confusing/error prone now that we have both assetCode
and assetCodeRaw
?
I propose that we just define assetCode
(without padding) in the config and then have a convenience function that derives the Pendulum asset from that. This should be the only place where we need to pad the assetCode
with the zero byte (?)
@@ -49,7 +49,7 @@ exports.subsidizePostSwap = async (req, res) => { | |||
const assetIssuerHex = `0x${Keypair.fromPublicKey(assetIssuer).rawPublicKey().toString('hex')}`; | |||
const pendulumCurrencyId = { | |||
Stellar: { | |||
AlphaNum4: { code: assetCode.padEnd(4, '\0'), issuer: assetIssuerHex }, | |||
AlphaNum4: { code: assetCodeRaw.padEnd(4, '\0'), issuer: assetIssuerHex }, |
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.
Isn't the idea that we don't need .padEnd(4, '\0')
for assetCodeRaw
?
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.
Yes you're right here, should remove the pad (which should have never been needed, we were already padding on the config).
@@ -202,7 +202,7 @@ function checkBalancePeriodically( | |||
try { | |||
const someBalanceUnits = await getStellarBalanceUnits( | |||
stellarTargetAccountId, | |||
outputToken.stellarAsset.code.stringRaw, |
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.
This one was wrong to have been raw
in the first place, as it compares with the return of the stellar balance query.
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.
Thanks for implementing the change requests!
No description provided.