This repository has been archived by the owner on Nov 27, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 5
feat: code-based instance configuration support #492
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Yeah I agree. If you look at past commits this was my initial approach. That being said I think this works fine for now and doesn't have any downside besides needing to run two scripts instead of one. |
dd0sxx
previously approved these changes
Nov 9, 2023
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.
looks great!
0xrajath
reviewed
Nov 9, 2023
0xrajath
reviewed
Nov 9, 2023
0xrajath
reviewed
Nov 9, 2023
0xrajath
reviewed
Nov 9, 2023
0xrajath
reviewed
Nov 9, 2023
0xrajath
reviewed
Nov 9, 2023
0xrajath
reviewed
Nov 9, 2023
0xrajath
reviewed
Nov 9, 2023
0xrajath
reviewed
Nov 9, 2023
0xrajath
reviewed
Nov 9, 2023
Co-authored-by: Rajath Alex <[email protected]>
0xrajath
approved these changes
Nov 9, 2023
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!
dd0sxx
approved these changes
Nov 9, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Code-based instance configurations serve as an alternative to our standard JSON-based instance configuration. JSON-based configs will work in most use cases but they are limited when a more advanced setup is required. Code-based configs use the power of llama scripts to enable features such as creating strategies or accounts with multiple logic contracts and creating permissions based on dynamic conditions (e.g. deploying a guard and then using the deployed address as the target address).
User flow:
just deploy-instance
using something likeadvancedInstanceConfig.json
as the config file (required to have instant execution strategy as the first strategy and role 1 assigned to deployer)src/llama-scripts/LlamaInstanceConfigScriptTemplate.sol
and inherits fromLlamaInstanceConfigBase
. Contract is deployed and address is used in next step.run-configure-advanced-instance-script
in thejustfile
and callsjust configure-advanced-instance
Modifications:
ConfigureAdvancedLlamaInstance.s.sol
script that can be run for an instance deployed using an advanced configuration (instant execution strategy on setRolePermission with a single policyholder).LlamaInstanceConfigScript.sol
which can be edited to describe the instance configuration using code. It also includes the post configuration cleanup that removes any trace of the config bot.Result:
Instances can be deployed using a standard JSON-based configuration or an advanced code-based configuration.