Skip to content
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

refactor: remove 0 check in recovered address in BatcherPaymentService #1064

Open
wants to merge 2 commits into
base: staging
Choose a base branch
from

Conversation

MarcosNicolau
Copy link
Collaborator

@MarcosNicolau MarcosNicolau commented Sep 24, 2024

Changes
This PR removes the check for zero addresses in the recovered signature within the BatcherPaymentService contract. The library handling the signature recovery already performs this check when splitting the fields (r, v, s) first and then calling the ecrecover precompile, where it does the check (see here).

Testing
I've tested that everything works by setting up a local environment and sending some sp1 and plonk proofs. Here is the process:

  1. Start aggregator: make aggregator_start.
  2. Start operator: make operator_register_and_start.
  3. Start batcher: make batcher_start_local.
  4. Send some proofs and make sure everything works as expected.

Contracts have been updated with make anvil_deploy_aligned_contracts.

Closes #1061

@MarcosNicolau MarcosNicolau changed the title refactor: remove check in BatcherPaymentService refactor: remove 0 check in recovered address in BatcherPaymentService Sep 24, 2024
Copy link

github-actions bot commented Sep 24, 2024

Changes to gas cost

Generated at commit: d8d846c25009e143caebf4976dc84642e84b428e, compared to commit: 4b2fabe8efd54cc36a600f08767aaaeec90cd991

🧾 Summary (10% most significant diffs)

Contract Method Avg (+/-) %
AlignedLayerServiceManager createNewTask +318 ❌ +0.43%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
AlignedLayerServiceManager 4,629,415 (0) createNewTask
receive
53,907 (+84)
21,169 (0)
+0.16%
0.00%
73,971 (+318)
44,970 (+373)
+0.43%
+0.84%
73,975 (+6)
45,064 (0)
+0.01%
0.00%
74,746 (0)
45,064 (0)
0.00%
0.00%
256 (0)
256 (0)

@MarcosNicolau MarcosNicolau self-assigned this Sep 24, 2024
Copy link
Contributor

@MauroToscano MauroToscano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-deploy aligned contracts and test it, and push them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants