feat(sessions): updating the cosign signing and the bundler #715

geekbrother · 2024-07-22T11:53:00Z

Description

This PR is making the following changes to the Co-Signing endpoint:

Changing the signature to comply with the Ethereum signing flow, using the LocalWallet and packing the signature.
Changing the bundler function and variables to not be sticked to the Biconomy (BICONOMY_BUNDLER_* -> BUNDLER_*) and changing the default bundler to the Pimlico.
Changing the request userOperation struct to be v07 from v06 to proper calculation of the packed UserOperation.
Adding calculation of the additional fields (paymaster, factory) for the packed UserOperation structure builder to support paymasters.
Multiple fixes and crypto utils functions refactors.

How Has This Been Tested?

This flow was tested manually with the testing dapp cosigning flow.

Due Diligence

Breaking change
Requires a documentation update
Requires a e2e/integration test update

geekbrother · 2024-07-22T11:55:03Z

src/handlers/identity.rs

@@ -466,8 +466,7 @@ impl JsonRpcClient for SelfProvider {
        let id = SystemTime::now()
            .duration_since(UNIX_EPOCH)
            .expect("Time should't go backwards")
-            .as_millis()
-            .to_string();
+            .as_secs();


We have a shared JsonRPC request struct and the bundler complained that the Id should be the int.

I'm not a fan of that expect outside of initialization code. Would you rather make it an error instead?

Makes sense. We are using this in many places so this should be a follow-up. #716 was created for this. Thanks for catching this!

geekbrother · 2024-07-22T12:02:31Z

src/utils/crypto.rs

-pub fn add_eip191(message: &str) -> String {
-    format!("\x19Ethereum Signed Message:\n{}{}", message.len(), message)
+/// Convert message to EIP-191 compatible format
+pub fn to_eip191_message(message: &[u8]) -> Vec<u8> {


Changing this function to use bytes instead of Strings, to remove double conversions since we are using bytes for the keccak256 hashing.

Nice refactor

geekbrother · 2024-07-22T12:03:16Z

src/utils/crypto.rs

+}
+
+/// Pack signature into a single byte array to Ethereum compatible format
+pub fn pack_signature(unpacked: &EthSignature) -> Bytes {


Packing of s,r,v of the ecdsa signature into the Ethereum format.

geekbrother · 2024-07-22T12:11:03Z

src/handlers/sessions/cosign.rs

-    // Update the userOp with the signature
-    user_op.signature = get_signature_result;
+    // Todo: remove this debug line before production stage
+    info!("UserOpPacked final JSON: {:?}", serde_json::json!(user_op));


We need this userOp json to trace it in case of issues for the alpha version. Since there is no any traffic to this endpoint, there shouldn't be a problem with the Cloudwatch cost.

chris13524 · 2024-07-22T20:09:45Z

src/handlers/sessions/cosign.rs

+            .context
+            .permissions_context
+            .clone()
+            .trim_start_matches("0x"),


Can we avoid this trim by having the original data or the client-facing API not provide this? Should be normalized one way or the other vs being permissive here and trimming it

The client sends this as a String representation of eth hex bytes "0x...", to pass this to the contract we need to convert this representation into ethers::Bytes as the contract expects the Bytes format. To do this we need to hex decode it and trim 0x to decode. That's why we need to trim 0x when converting it.

chris13524 · 2024-07-22T20:12:48Z

src/utils/crypto.rs

+/// Convert message to EIP-191 compatible format
+pub fn to_eip191_message(message: &[u8]) -> Vec<u8> {
+    let prefix = format!("\x19Ethereum Signed Message:\n{}", message.len());
+    let mut eip191_message = Vec::with_capacity(prefix.len() + message.len());


Nice using with_capacity

chris13524 · 2024-07-22T20:13:51Z

src/utils/crypto.rs

+    r.to_big_endian(&mut r_bytes);
+    s.to_big_endian(&mut s_bytes);
+    // Pack r, s, and v into a single byte array
+    let mut packed_signature = Vec::with_capacity(65);


Self-documenting where what the size is based on

Suggested change

let mut packed_signature = Vec::with_capacity(65);

let mut packed_signature = Vec::with_capacity(r_bytes.len() + s_bytes.len() + 1);

Makes sense! Changed it. Thanks.

chris13524

LGTM

We'll need tests here soon!

geekbrother · 2024-07-23T08:36:26Z

We'll need tests here soon!

Totally! I'm testing by the manual run of the integration test now, since we need a gas to make the full flow.
I'm thinking to reduce the test to just the validation/simulation step for the CI/CD integration tests.

geekbrother self-assigned this Jul 22, 2024

geekbrother temporarily deployed to staging July 22, 2024 11:54 — with GitHub Actions Inactive

geekbrother temporarily deployed to prod July 22, 2024 11:54 — with GitHub Actions Inactive

geekbrother commented Jul 22, 2024

View reviewed changes

feat(sessions): updating the cosign signing and the bundler

36f02ed

geekbrother force-pushed the fix/co-signer_signature_bundler_updates branch from eb01bed to 36f02ed Compare July 22, 2024 12:00

geekbrother temporarily deployed to staging July 22, 2024 12:01 — with GitHub Actions Inactive

geekbrother temporarily deployed to prod July 22, 2024 12:01 — with GitHub Actions Inactive

geekbrother commented Jul 22, 2024

View reviewed changes

geekbrother requested review from chris13524 and nopestack July 22, 2024 12:05

geekbrother temporarily deployed to prod July 22, 2024 12:06 — with GitHub Actions Inactive

geekbrother temporarily deployed to staging July 22, 2024 12:06 — with GitHub Actions Inactive

geekbrother commented Jul 22, 2024

View reviewed changes

geekbrother marked this pull request as ready for review July 22, 2024 12:12

nopestack approved these changes Jul 22, 2024

View reviewed changes

chris13524 reviewed Jul 22, 2024

View reviewed changes

chris13524 approved these changes Jul 22, 2024

View reviewed changes

geekbrother temporarily deployed to staging July 23, 2024 10:45 — with GitHub Actions Inactive

geekbrother temporarily deployed to prod July 23, 2024 10:45 — with GitHub Actions Inactive

fix: the signature length changed

ee8a1fa

geekbrother force-pushed the fix/co-signer_signature_bundler_updates branch from 47d9a34 to ee8a1fa Compare July 23, 2024 11:01

geekbrother temporarily deployed to prod July 23, 2024 11:03 — with GitHub Actions Inactive

geekbrother temporarily deployed to staging July 23, 2024 11:03 — with GitHub Actions Inactive

geekbrother merged commit d91a40f into master Jul 23, 2024
16 checks passed

geekbrother deleted the fix/co-signer_signature_bundler_updates branch July 23, 2024 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sessions): updating the cosign signing and the bundler #715

feat(sessions): updating the cosign signing and the bundler #715

geekbrother commented Jul 22, 2024 •

edited

Loading

geekbrother Jul 22, 2024

nopestack Jul 22, 2024

geekbrother Jul 23, 2024

geekbrother Jul 22, 2024

chris13524 Jul 22, 2024

geekbrother Jul 22, 2024

geekbrother Jul 22, 2024

chris13524 Jul 22, 2024

geekbrother Jul 23, 2024

chris13524 Jul 22, 2024

chris13524 Jul 22, 2024 •

edited

Loading

geekbrother Jul 23, 2024

chris13524 left a comment

geekbrother commented Jul 23, 2024

	let mut packed_signature = Vec::with_capacity(65);
	let mut packed_signature = Vec::with_capacity(r_bytes.len() + s_bytes.len() + 1);

feat(sessions): updating the cosign signing and the bundler #715

feat(sessions): updating the cosign signing and the bundler #715

Conversation

geekbrother commented Jul 22, 2024 • edited Loading

Description

How Has This Been Tested?

Due Diligence

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris13524 Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris13524 left a comment

Choose a reason for hiding this comment

geekbrother commented Jul 23, 2024

geekbrother commented Jul 22, 2024 •

edited

Loading

chris13524 Jul 22, 2024 •

edited

Loading