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 allowlist deploy script & fix existing deploy script [2/2] #127

Merged
merged 4 commits into from
Aug 2, 2024

Conversation

adamegyed
Copy link
Contributor

Motivation

To test the use of permission-limiting hooks, we should deploy AllowlistModule. To do that, we need a deploy script.

Also I found some issues in the existing script + factory setup, so fixing those here, too.

Solution

  • Add a linter step and config for scripts
  • Fix a bug in Deploy.s.sol where the sender was staking with the EntryPoint, instead of the factory.
    • Add a check for this behavior in the script's test Deploy.s.t.sol.
  • Switch Deploy.s.sol from console2 to console - there's no difference anymore.
  • Add a deploy script for AllowlistModule and a test for the deploy script.
  • Update AccountFactory to take in the unstake delay from the owner's call, rather than specifying this as an immutable.

@adamegyed adamegyed requested a review from a team August 1, 2024 21:07
@adamegyed adamegyed force-pushed the adam/allowlist-deploy branch from 68a95cc to 99d5a55 Compare August 1, 2024 21:13
uint256 stakeToAdd = stakeAmount - currentStake;

if (stakeToAdd > 0) {
console2.log("Adding stake: ", stakeToAdd);
entryPoint.addStake{value: stakeToAdd}(unstakeDelay);
Copy link
Collaborator

Choose a reason for hiding this comment

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

: |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha this comment made me realize a mistake - we should use the value stakeToAdd, not stakeAmount

@fangting-alchemy
Copy link
Collaborator

Look good. Need to fix tests

@adamegyed adamegyed force-pushed the adam/allowlist-deploy branch from 99d5a55 to 4dc064e Compare August 1, 2024 21:40
@adamegyed adamegyed force-pushed the adam/allowlist-update branch from 5aa0d70 to d3922e2 Compare August 1, 2024 23:08
@adamegyed adamegyed force-pushed the adam/allowlist-deploy branch 2 times, most recently from afc6423 to 6ddbf5b Compare August 2, 2024 15:11
@adamegyed adamegyed force-pushed the adam/allowlist-update branch from 0886e60 to ce25904 Compare August 2, 2024 18:01
@adamegyed adamegyed force-pushed the adam/allowlist-deploy branch from 6ddbf5b to 9a49d23 Compare August 2, 2024 18:03
Base automatically changed from adam/allowlist-update to develop August 2, 2024 18:14
@adamegyed adamegyed merged commit c4e81b5 into develop Aug 2, 2024
3 checks passed
@adamegyed adamegyed deleted the adam/allowlist-deploy branch August 2, 2024 18:14
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.

2 participants