-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: Adds new tx contract data model [REFACTOR] #102
base: abhk/hex
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
PR Summary
This PR refactors the transaction handling system to support the new Hex bridge API, replacing the soon-to-be-sunset bridge indexer across all networks.
- Removed
bridgeIndexerBaseUrl
from config and updatedbridgeApiBaseUrl
to point to the new production sandbox URL - Added new
TransactionResponseType
enum intypes/common.ts
withAVAIL_SEND
andETH_SEND
values - Significantly restructured the
Transaction
interface by removing chain-specific fields and consolidating token address fields into a singletokenId
- Critical issue in
validateParams
function inservices/transactions.ts
where it returns an empty array in error cases instead of properly handling errors
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
4 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
function validateTransactionResponseType(queryParam: string) : string { | ||
const chainAddressResponseMapping: Record<string, TransactionResponseType> = Object.freeze({ | ||
"ethAddress": TransactionResponseType.ETH_SEND, | ||
"availAddress": TransactionResponseType.AVAIL_SEND | ||
}) | ||
return chainAddressResponseMapping[queryParam]; | ||
} |
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.
style: This function doesn't handle the case where queryParam is not one of the keys in chainAddressResponseMapping, which could lead to undefined being returned.
function validateTransactionResponseType(queryParam: string) : string { | |
const chainAddressResponseMapping: Record<string, TransactionResponseType> = Object.freeze({ | |
"ethAddress": TransactionResponseType.ETH_SEND, | |
"availAddress": TransactionResponseType.AVAIL_SEND | |
}) | |
return chainAddressResponseMapping[queryParam]; | |
} | |
function validateTransactionResponseType(queryParam: string) : TransactionResponseType { | |
const chainAddressResponseMapping: Record<string, TransactionResponseType> = Object.freeze({ | |
"ethAddress": TransactionResponseType.ETH_SEND, | |
"availAddress": TransactionResponseType.AVAIL_SEND | |
}) | |
const type = chainAddressResponseMapping[queryParam]; | |
if (!type) { | |
throw new Error(`Invalid query parameter: ${queryParam}`); | |
} | |
return type; | |
} |
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 fact early return/throws validation is way better.
e.g:
#L28 if !["",""].includes(queryParams) throw new Error(`Invalid query parameter: ${queryParam}`)
WIP DON'T MERGE
Will later adjust frontend layer.
Will do some local tests over the week.
Changes
Reason for change