-
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
Route Book #93
Route Book #93
Conversation
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.
Lots of comments, but feel free to merge after addressing
interface Params { | ||
symbol1: string; | ||
symbol2: string; | ||
hops: string; | ||
limit: string; | ||
} |
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.
idea: not for this PR, but think trpc would be good for us to use
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.
@grod220 @JasonMHasperhoven wouldn't it be easier to not use the api
directory but a simple server-side fetch within a component? Like
export default async function Page() {
let data = await fetch('https://api.vercel.app/blog')
let posts = await data.json()
return (
<ul>
{posts.map((post) => (
<li key={post.id}>{post.title}</li>
))}
</ul>
)
}
Instead of the fetch
from my example, we could write useAsksBids()
from the API made by Jason. It would do all the server-side work while sharing the types eliminating the need in TRPC. Wdyt?
My previous comment about this: #75 (comment)
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.
Let's discuss this in our next call, this is frankly new to me.
const hops = Number(hopsParam); | ||
const limit = Number(limitParam); | ||
|
||
const registry = await chainRegistryClient.remote.get(process.env['PENUMBRA_CHAIN_ID'] ?? ''); |
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.
suggestion: if env vars are not correctly injected, think this will produce a strange error. Suggest:
const chainId = process.env['PENUMBRA_CHAIN_ID'];
if (!chainId) {
throw new Error('No chain id in env vars');
}
const registry = await chainRegistryClient.remote.get(chainId);
const sellSidePair = new DirectedTradingPair({ | ||
start: metadata2?.penumbraAssetId, | ||
end: metadata1?.penumbraAssetId, | ||
}); |
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.
suggestion: we should probably type guard for this too right (to prevent strange error downstream)?
if (!metadata1?.penumbraAssetId || !metadata2?.penumbraAssetId) {
throw new Error(`no asset id for ${symbol1} or ${symbol2}`);
}
const data = await Promise.all([ | ||
querier.liquidityPositionsByPrice(sellSidePair, hops), | ||
querier.liquidityPositionsByPrice(buySidePair, hops), | ||
]).then(([asks, bids]) => ({ | ||
asks: asks?.slice(0, limit), | ||
bids: bids?.slice(0, limit), | ||
})); |
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.
suggestion: I find it more readable if we don't mix async/await & then()
const [asks, bids] = await Promise.all([
querier.liquidityPositionsByPrice(sellSidePair, hops),
querier.liquidityPositionsByPrice(buySidePair, hops),
]);
const data = {
asks: asks?.slice(0, limit),
bids: bids?.slice(0, limit),
};
src/components/route-book/index.tsx
Outdated
data.reduce((displayData: Record<string, RouteWithTotal>, route: Route) => { | ||
const key = route.price; | ||
return { | ||
...displayData, |
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.
thought: I wonder if this spread, creating a new object on every loop, can be inefficient on large data sets. Maybe not a concern. If so though, can do something like const displayData = new Map<number, RouteWithTotal>();
with a normal for loop.
export const useBook = ( | ||
symbol1: string | undefined, | ||
symbol2: string | undefined, | ||
hops: number | undefined, | ||
limit: number | 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.
idea: Not critical to this PR, but starting to realize we'll want to include block height in the query key with nearly all our queries (as they'll get invalidated every block and refetched). Maybe something for later.
src/fetchers/book.ts
Outdated
}; | ||
} | ||
const res = await fetch(`/api/book/${symbol1}/${symbol2}/${hops}/${limit}`); | ||
return (await res.json()) as BookResponse; |
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.
thought: This is where trpc would be helpful
src/shared/useComputePositionId.ts
Outdated
|
||
useEffect(() => { | ||
const set = async () => { | ||
// cant import directly without breaking the build cmd |
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.
question: why does that happen I wonder. Is it because the wasm file is made for web and not nodejs?
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 don't know the reason tbh..
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.
Some final suggestions, but feel free to merge after
src/components/route-book/index.tsx
Outdated
fromBaseUnit(BigInt(property?.lo ?? 0), BigInt(property?.hi ?? 0), exponent); | ||
|
||
const routes = data | ||
// .filter(position => position.state?.state.toLocaleString() === 'POSITION_STATE_ENUM_OPENED') |
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.
nit: remove
// .filter(position => position.state?.state.toLocaleString() === 'POSITION_STATE_ENUM_OPENED') | ||
.filter(position => position.state?.state === PositionState_PositionStateEnum.OPENED) | ||
.map(position => { | ||
const direction = position.phi?.pair?.asset2?.equals(asset1.penumbraAssetId) ? -1 : 1; |
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.
question: does -1
and 1
have any inherent meaning within trade routing? If not, think it would be easier to make this a string label like primary
and secondary
or something.
const asset1Exponent = asset1 ? getDisplayDenomExponent(asset1) : 0; | ||
const asset2Exponent = asset2 ? getDisplayDenomExponent(asset2) : 0; | ||
const { data: computePositionId } = useComputePositionId(); | ||
const { data } = useBook(asset1?.symbol, asset2?.symbol, 100, 50); |
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.
suggestion: related to my other comment, but I think we should consider:
if (!asset1?.symbol || !asset2?.symbol) {
return <div>No asset selected</div>
}
Note that this comes before useBook()
, and given the rule of hooks, you'd have to create another component after the if clause.
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 see your point, if you don't mind I will address this in the next update (separate pr).
symbol1: string | undefined, | ||
symbol2: string | 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.
suggestion: think these should be required fields
return useQuery({ | ||
queryKey: ['book', symbol1, symbol2, hops, limit], | ||
queryFn: async (): Promise<BookResponse> => { | ||
if (!symbol1 || !symbol2 || !limit) { |
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.
question: limit is a required field too? If so, think we should change the type signature of the hook.
const asset1 = assets?.find(asset => asset.symbol === 'UM'); | ||
const asset2 = assets?.find(asset => asset.symbol === 'GM'); |
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.
suggestion: use pair
MobX state (src/shared/state/pair.ts
) to get the asset info. You may also fill this state initially, so from
and to
won't be 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 doesn't seem to work for me, the asset selector doesn't seem to fire on click?
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.
That's weird, it works fine for me... Would love if you can make the reproduction
select.mp4
bb2cf1a
to
d122e13
Compare
Implements #64
With very basic UI (and without mid price)