-
Notifications
You must be signed in to change notification settings - Fork 112
Feat/bridge price shape #221
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
Conversation
size-limit report 📦
|
Velenir
left a 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.
- Shape changed, a bunch of tests that use MatchSnapshot broke. Change API_URL when running tests to see
- Since you are changing types, could you add
maxImpactparam togetQuotemethod? It was added in API at some point recently.
| | 'receivedDestAmount' | ||
| | 'receivedDestAmountBeforeFee' | ||
| | 'receivedDestUSD' | ||
| | 'receivedDestUSDBeforeFee' |
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.
Could you check if srcAmountBeforeFee and other props specific for BUY are also added?
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, availableBridges doesn't include src* stuff
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.
should it though?
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.
All the src* props would be the same across all bridges in availableBridges. I don't know if it's worth increasing price response by duplicating them, but on the other hand it wouldn't be bad to have AvailablePrice equal to BridgePrice.
src/methods/delta/getBridgeInfo.ts
Outdated
| export type BridgeProtocolResponse = { | ||
| protocol: string; | ||
| displayName: string; | ||
| icon: string; // CDN URL |
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 necessarily CDN, let's not limit ourselves 😉
Added 1216a51 |
I fixed the snapshot, but a bunch of tests are still failing, though none of them are related to my change (I hope) |
| /** @description Partner string */ | ||
| partner?: string; | ||
| /** @description Maximum price impact (in percentage) acceptable for the trade */ | ||
| maxImpact?: number; |
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.
maxImpactUSD?
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.
Somehow I didn't notice it
Added 2936ff5
|
Could you also replace wherever we make requests with ?network= to ?chainId=. they all now work with plain |
Done 45018d2 |
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Updates BridgePrice types
Note
BridgePriceInfonow includesfastest,bestReturn,recommendedflagsAvailableBridgerefactored to includebridgeandbridgeInfo; carriesreceivedDest*fieldsAvailableBridgeand updates SDK surface insrc/index.tsBridgeProtocolResponsenow includesicongetQuoteacceptsmaxImpactandmaxUSDImpactnetworktochainIdinadapters,rates,spender,swapTxand update related docs/comments9.3.1Written by Cursor Bugbot for commit 8195608. This will update automatically on new commits. Configure here.