-
Notifications
You must be signed in to change notification settings - Fork 13
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: EIP-1271 support #55
Conversation
19eca03
to
685200f
Compare
use {super::get_rpc_url::GetRpcUrl, crate::domain::ProjectId, url::Url}; | ||
|
||
// https://github.com/WalletConnect/blockchain-api/blob/master/SUPPORTED_CHAINS.md | ||
const SUPPORTED_CHAINS: [&str; 26] = [ |
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 considered checking the response of Blockchain API to determine support or not, but this would make the CACAO verification function more coupled to Blockchain API so decided to hardcode the supported list here for now.
In the future we should either pull the list dynamically, or refactor the CACAO to use a more generic error handling and be able to interpret the response code. I slightly prefer the check-before-request approach to avoid redundant requests.
Captured in this issue: WalletConnect/notify-server#345 (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.
LGTM
|
||
#[derive(Debug, Clone)] | ||
pub struct BlockchainApiProvider { | ||
project_id: ProjectId, |
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.
Are we using NotifyServer and Keys Server project_ids? Might be good to add it to allowlist to not have rate limiting there
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.
Good call out!
This should be fine for now, as 1 request per second is 86k/day. Limit/day is 100k. Notify Server does 1-2 msgs/s and Keys Server does 0.1/s.
Also most people have EOAs not smart accounts. We'll add metrics soon for this so we can see more details.
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.
Also we have metrics in Cloud already for RPC usage, so we should see this too as-is.
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.
LGTM!
Description
Validates EIP-1271 (smart account) signatures via a dynamic provider.
How Has This Been Tested?
EIP-191 tested with existing automated tests.
EIP-1271 tested manually with an Argent wallet. There is an
#[ignored]
test you can substitute with values if you would like to test yourself, but I don't want to dox my wallet here. Not doing automated tests for now as it's complex to start a blockchain node, deploy a mock smart contract, and execute it. I want[ed] to do this, but hit too many walls trying to get foundry and forge to run from a cargo test. Opened an issue here for this.Due Diligence