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

feat: add onlyAggregator to respondToTask #883

Conversation

uri-99
Copy link
Contributor

@uri-99 uri-99 commented Aug 28, 2024

Note

Merge after #875
This branch already has 875 contents, and is running on STAGE

This PR

onlyAggregator modifier was added to respondToTask.
For this, address aggregator was added on storage (and GAP decreased)

Implementation

To implement this contract upgrade, the same structure of implement pausable PR was followed;

  • A new solidity stored variable was created, alignedAggregator.
  • This variable is set on a reinitializer(2) function, as the industry standard.
  • The variable is also set on the first initialize of the contract, to avoid having to run the reinitialize on a freshly deployed contract (i.e mainnet). This doesn't change the current testnet deploys as this function can not be ran again, thus the reinitializer(2) function.
  • 2 new make targets were made, to deploy in devnet and testnet.

Deploying

Devnet:

make anvil_upgrade_add_aggregator

Testnet:

  • Stage or prod variables should be set accordingly on the contracts/scripts/.env file
  • Config variables should be set accordingle on: contracts/script/deploy/config/holesky/aligned.holesky.config.json
make upgrade_add_aggregator

Testing this PR

  1. Deploy previous version of contracts:
git checkout main
make anvil_deploy_aligned_contracts
make anvil_start_with_block_time

The following call should revert:

cast call 0x1613beB3B2C4f22Ee086B2b38C1476A3cE7f78E8 "alignedAggregator()(address)" --rpc-url 127.0.0.1:8545

But the following call should not revert:
(this is just to verify we have the correct ServiceManager address)

cast call 0x1613beB3B2C4f22Ee086B2b38C1476A3cE7f78E8 "balanceOf(address)(uint256)" 0x1613beB3B2C4f22Ee086B2b38C1476A3cE7f78E8 --rpc-url 127.0.0.1:8545

Then close anvil

ctrl + c

You must preserve this anvil state before you switch to the new branch, for this you can:

git stash
git checkout 851-add-max-limit-eth-in-payment-from-batcher-to-aggregator-make-the-respond-to-ask-only-callable-by-one-agg
git apply
  1. Upgrade to new version of contracts (make sure you are using previous anvil-state !)

The following upgrade should run and work without problems:

make anvil_upgrade_add_aggregator

Then you must start the devnet

make anvil_start_with_block_time

And the following call should return the aggregator's address:

cast call 0x1613beB3B2C4f22Ee086B2b38C1476A3cE7f78E8 "alignedAggregator()(address)" --rpc-url 127.0.0.1:8545

Testing the modifier

To make sure the onlyAggregator modifier actually works, you must:

  • save a backup of anvil.aggregator.ecdsa.key.json
  • copy (for example) operator-3.ecdsa.key.json with the name anvil.aggregator.ecdsa.key.json, where this was before
  • run aggregator, operator, batcher.
make batcher_send_risc0_burst

Aggregator should revert when trying to submit:

Error: execution reverted: custom error 2cbe4195:00000000000000000000000090f79bf6…1aad88f6f4ce6ab8827279cfffb92266 (64 bytes)

uri-99 and others added 30 commits August 26, 2024 15:59
Copy link

github-actions bot commented Sep 3, 2024

Changes to gas cost

Generated at commit: e7ddbd8eb9b8f0e3010b9be269eb2452f013320c, compared to commit: 81b290dbed7aa25420674fc138c776751b8758cb

🧾 Summary (10% most significant diffs)

Contract Method Avg (+/-) %
AlignedLayerServiceManager batchesState
createNewTask
+66 ❌
-22 ✅
+12.11%
-0.04%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
AlignedLayerServiceManager 4,623,742 (+99,539) batchesState
createNewTask
611 (+66)
51,138 (-22)
+12.11%
-0.04%
611 (+66)
51,303 (-22)
+12.11%
-0.04%
611 (+66)
51,222 (-22)
+12.11%
-0.04%
611 (+66)
51,864 (-22)
+12.11%
-0.04%
256 (0)
256 (0)

Copy link
Contributor

@NicolasRampoldi NicolasRampoldi left a comment

Choose a reason for hiding this comment

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

I have done the following steps indicated in the PR description:

  1. Checkout to main.
  2. Deploy Aligned contracts.
  3. Use that anvil state in this branch.
  4. Upgrade.
  5. Call the contract.

But I'm getting transaction reverted

@entropidelic
Copy link
Contributor

same comment as @NicolasRampoldi on my side too

@uri-99
Copy link
Contributor Author

uri-99 commented Sep 4, 2024

I have fixed the steps to test this PR , the contract had a refactor renaming the aggregator variable to alignedAggregator , so the call to get this variable is cast call 0x... alignedAggregator()(address)

Copy link
Collaborator

@JuArce JuArce left a comment

Choose a reason for hiding this comment

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

LGTM. Ready for test in stage

Copy link
Collaborator

@JuArce JuArce left a comment

Choose a reason for hiding this comment

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

lgtm. The contract is deployed on stage

@entropidelic entropidelic merged commit e282e12 into main Sep 5, 2024
3 checks passed
@entropidelic entropidelic deleted the 851-add-max-limit-eth-in-payment-from-batcher-to-aggregator-make-the-respond-to-ask-only-callable-by-one-agg branch September 5, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: make the respond to task only callable by one AGG
5 participants