Refactor solve form #26
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| useEffect(() => { | ||
| if (!formValues.sourceChain && chains.length > 0) { | ||
| const chainID = | ||
| localStorage.getItem("IBC_DOT_FUN__LAST_SOURCE_CHAIN") ?? "cosmoshub-4"; |
There was a problem hiding this comment.
We should probably define these const names in a consts file or at the top of this file and just refer to them here, rather than needing to refer to the raw text multiple times
| useEffect(() => { | ||
| if (formValues.sourceChain) { | ||
| localStorage.setItem( | ||
| "IBC_DOT_FUN__LAST_SOURCE_CHAIN", |
| sourceAsset: feeAsset, | ||
| })); | ||
| } else { | ||
| const assets = assetsByChainID(formValues.sourceChain.chain_id); |
There was a problem hiding this comment.
This should be modified to make sure it only returns native assets -- and ideally only assets that include metadata.
For the first part, you can use the native_only field in the /assets request. For the second part, we need to implement all_assets first and only getting tokens with metadat will be the default behavior: https://linear.app/skip/issue/ENG-1582/add-all-assets-parameter-to-assets
There was a problem hiding this comment.
This is already handled elsewhere
There was a problem hiding this comment.
| destinationAsset: feeAsset, | ||
| })); | ||
| } else { | ||
| const assets = assetsByChainID(formValues.destinationChain.chain_id); |
There was a problem hiding this comment.
Same comment I made for source assets. Also feels like this logic could be abstracted to a helper function since it's duplicated
| {!route.does_swap && | ||
| `Successfully transfered ${ | ||
| sourceAsset?.symbol ?? route.source_asset_denom | ||
| } from ${sourceChain.prettyName} to ${destinationChain.prettyName}`} |
There was a problem hiding this comment.
Where are you getting pretty name from? LMK if this is coming from somewhere outside of the API
There was a problem hiding this comment.
Looks like it comes from chain-registry, our chains endpoint has a chain-name but it's not capitalized.
I assume he is using it this way because the chain registry one is easy to access.
|
|
||
| const msgsResponse = await skipClient.fungible.getMessages(msgRequest); | ||
|
|
||
| // check balances on chains where a tx is initiated |
There was a problem hiding this comment.
good helper function opportunity
| } | ||
|
|
||
| for (let i = 0; i < msgsResponse.msgs.length; i++) { | ||
| const multiHopMsg = msgsResponse.msgs[i]; |
There was a problem hiding this comment.
Also good helper opportunity
| ], | ||
| { | ||
| amount: [coin(0, feeInfo.denom)], | ||
| gas: `${simulatedGas * 1.2}`, |
There was a problem hiding this comment.
Move this number to a well named constant var
| } | ||
|
|
||
| if (multiHopMsg.chain_id === "evmos_9001-2") { | ||
| const tx = await signAndBroadcastEvmos(walletClient, msgJSON.sender, { |
There was a problem hiding this comment.
Why do these have custom logic? Do we need to highlight this to users in our docs?
|
|
||
| export async function getNumberOfTransactionsFromRoute(route: RouteResponse) { | ||
| const userAddresses: Record<string, string> = {}; | ||
| for (const chainID of route.chain_ids) { |
|
Added a few comments, agree with Barry's comments. |
Refactors form logic to remove a lot of left over tech debt from the previous non-unified API.
🎉 Also adds tests 🎉