-
Notifications
You must be signed in to change notification settings - Fork 590
fix:swap api #817
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
fix:swap api #817
Conversation
✅ Heimdall Review Status
|
|
Hi @0xRAG, I rebased to account for your changes in #818. I still can't get the swap API to work with smart accounts, it throws this error Also tried to use createSwapQuote + sign permit2 + sendTransaction instead of the all-in-one pattern.
Any ideas how solve this? |
|
Hey @phdargen, I think this may happen when the smart wallet owner is not a CDP wallet, since Could you confirm by trying again with a new smart wallet whose owner is a CDP server wallet? We may need to add a disclaimer that swap only works with smart accounts owned by a CDP wallet. |
3dca4bb to
f7332e9
Compare
Hi @0xRAG, thanks that was indeed the problem! I thus changed the chatbot and next template to create a CDP server wallet as owner by default and the swap action throws a descriptive error when its called with a LocalAccount as owner |
| ? walletProvider.smartAccount | ||
| : await walletProvider.getClient().evm.getAccount({ | ||
| address: walletProvider.getAddress() as Hex, | ||
| }); |
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.
Given we are getting and later using the account / smart account via .swap, I think this action is better suited to live in the cdpEvmWalletActionProvider / cdpSmartWalletActionProvider. The get_swap_price action is provider-agnostic so that one is fine to remain, although may be worth moving along with swap just to reduce potential confusion on why the actions are on different providers. LMK your thoughts, also cc @CarsonRoscoe
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.
yeah good point, this would avoid the messy conditional logic and type casts. Will refactor
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.
Based on the codebase today, I am in agreement.
However, this is making me think about a refactor. There seems to be an association between wallet providers & action providers, and I think that's causing some confusion here.
Imagine a world where wallet provider files exported their own action providers, or used the CreateAction() decorator on their own functions, allowing them to declare tools that we can auto-register when a wallet provider is used. This would create a clear separation of concerns between what actions are needed for a specific wallet, and what actions are coming from a shared API like the CDP API. It also would reduce a friction point in the devx, which is knowing that, for example, using the CdpEvmWalletProvider means importing both WalletActionProvider and CdpEvmWalletActionProvider. It then allows CdpApiActionProvider to remain pure and focus only on features that any wallet using CDP can leverage such as get_swap_price and faucet.
Description
Fixes various issues with the swap API implementation in CdpApiActionProvider
Other changes:
Tests
Checklist
A couple of things to include in your PR for completeness: