-
Notifications
You must be signed in to change notification settings - Fork 2
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(#328): my trades implementation #337
base: main
Are you sure you want to change the base?
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.
// TODO: filter out irrelevant swaps from pindexer | ||
const { data, isLoading, error } = useLatestSwaps(subaccount); |
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.
todo: pindexer might return 2+ swaps per asset pair and a block height. Only one swap must be filtered out to match the user's swap. Might need some help do to 2+ swaps in the same block
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 think it’s almost there! I left some comments with observations specific to my wallet
return []; | ||
} | ||
|
||
// Two requests to get swaps in both directions: buy and sell |
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.
ACK – rendering both swap directions of the pair.
* Combines the list of pair:height combinations into a list of unique pairs with all heights in a merged array. | ||
* This transformation is needed to reduce the amount of database queries. | ||
*/ | ||
const adaptBody = (body: MyExecutionsRequestBody[]): ExecutionCollection[] => { |
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.
ACK – taking the mapped swap objects and transforming them into a form more amenable for efficient pindexer querying.
const [registry, sellStream, buyStream] = await Promise.all([ | ||
registryClient.remote.get(chainId), | ||
Promise.all(data.map(data => pindexer.myExecutions(data.base, data.quote, data.heights))), | ||
Promise.all(data.map(data => pindexer.myExecutions(data.quote, data.base, data.heights))), | ||
]); |
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.
comment:
myTradesQuery
is returning 52 results for UM <> USDC pair (out of 64 swaps stored in IndexedDB).
myExecutionsQuery
then sends the mapped swap objects to the server endpoint (/api/my-executions
).
- The server then queries pindexer which filters these results down to only 5 swaps, which isn't correct. I think this is pointing to an incorrect SQL 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.
@TalDerei this might be related to the issue with pindexer not storing (or losing) some swaps, discussed in the protocol chat. Same happens to me with old swaps, the new ones should work fine
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.
@TalDerei this might be related to the issue with pindexer not storing (or losing) some swaps, discussed in the protocol chat. Same happens to me with old swaps, the new ones should work fine
I see, linked this discussion in that thread
@TalDerei I did a major refactor to the SQL query. Now it sends only 1 request to Pindexer instead of multiple ones and uses only Postgres features to perform the checks:
An important recent change: now the merge happens not only by pair and height, but also by the |
Closes #328
Waits for penumbra-zone/web#2019 and then Prax release
Considerations of this PR:
/api/my-executions
is a POST method that accepts a possibly large body + AssetIds instead of symbols to not search for it in the registry and save timeuseLatestSwaps
hook requests data in a pipeline, from Prax first, then from Pindexer/api/my-executions
will not be made to save timemy-trades.mp4
Before running: install local version of the
protobuf
package