-
Notifications
You must be signed in to change notification settings - Fork 66
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 #78: Decode warp route messages. #144
base: main
Are you sure you want to change the base?
Conversation
@spilehchiha is attempting to deploy a commit to the Abacus Works Team on Vercel. A member of the Team first needs to authorize it. |
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.
The general approach looks okay but needs some fixes please
@@ -105,6 +106,39 @@ export function ContentDetailsCard({ | |||
blurValue={blur} | |||
/> | |||
</div> | |||
{warpRouteDetails && ( |
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.
Please put this in its own card like the Interchain Gas Payments
card
@@ -151,6 +159,7 @@ function parseMessage( | |||
totalGasAmount: m.total_gas_amount.toString(), | |||
totalPayment: m.total_payment.toString(), | |||
numPayments: m.num_payments, | |||
warpRouteDetails: warpRouteDetails ?? 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.
this ?? undefined
doesn't do anything right?
import { postgresByteaToAddress } from '../features/messages/queries/encoding'; | ||
import { WarpRouteDetails } from '../types'; | ||
|
||
export function parseWarpRouteDetails( |
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.
Please move this to src/features/messages/queries/parse.ts
as that's the only place it's used.
I'm not a big fan of accumulating business logic in the utils
folders
metadata?: any | ||
): WarpRouteDetails | undefined { | ||
try { | ||
if (!messageBody?.trim()) throw new Error('Invalid message body'); |
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.
It's not necessarily an invalid body. Just return undefined
totalPayment: utils.formatEther(gasInfo.totalPayment.toString()) | ||
}; | ||
} catch (error) { | ||
console.error('Failed to parse token details:', error, 'Message body:', messageBody); |
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.
no console, use logger. Did the lint not raise this?
amount: utils.formatEther(parsedMessage.amount.toString()), | ||
totalPayment: utils.formatEther(gasInfo.totalPayment.toString()) |
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.
Please use the hyp utils here: https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/main/typescript/utils/src/amount.ts#L13
const parsedMessage = parseWarpRouteMessage(messageBody); | ||
|
||
return { | ||
token: originTx?.to ? postgresByteaToAddress(originTx.to, metadata) : 'Unknown Token', |
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.
Not a huge deal but reorder things in the calling function so we don't have to decode this to
value twice
This commit is a good base case for fixing #78.
Three edge cases remain to improve the UX:
Will work on this.