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

Add builder script again #19

Merged
merged 10 commits into from
May 16, 2023
Merged

Add builder script again #19

merged 10 commits into from
May 16, 2023

Conversation

Rubilmax
Copy link
Collaborator

@Rubilmax Rubilmax commented May 15, 2023

GitHub dropped my work last friday, so here is the true last version of this tool. I am angry.

This PR introduces the builder script, which is completely independent from the other scripts already shipped in this repository. The builder script has nodejs dependencies, but no direct interfaces with forge scripts are available. Instead, the builder script simply builds a tx.json for the user, who's free to use it or replace it with their own.

@Rubilmax Rubilmax marked this pull request as ready for review May 15, 2023 14:45
Makefile Outdated Show resolved Hide resolved
Copy link

@MathisGD MathisGD left a comment

Choose a reason for hiding this comment

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

I'm not a big fan of adding "a lot" of dependancies to this "critical" repo. I have in mind the arguments of the well known dependancies, of the yarn.lock, and of the fact that scripts are independent but idk I'm still a little bit less confortable when the dependancies goes to number that I can't quickly "audit". No veto though, if you think that the feature is really worth it.

PS: I didn't review the PR.

@Rubilmax
Copy link
Collaborator Author

I'm not a big fan of adding "a lot" of dependancies to this "critical" repo. I have in mind the arguments of the well known dependancies, of the yarn.lock, and of the fact that scripts are independent but idk I'm still a little bit less confortable when the dependancies goes to number that I can't quickly "audit". No veto though, if you think that the feature is really worth it.

PS: I didn't review the PR.

The feature solves the following issue:

  • it is difficult to build a multisend governance tx if Gnosis frontend is down

How would the Morpho DAO pause supply and borrow only for specific (or all) markets otherwise (genuinely asking)? Creating and signing N (nb of markets) txs?

We can also export the script to another repository, given it is completely independent from the tx signature scripts

@MerlinEgalite
Copy link
Collaborator

We can also export the script to another repository, given it is completely independent from the tx signature scripts

That a suggestion from @julien-devatom. Should we?

@Rubilmax
Copy link
Collaborator Author

We can also export the script to another repository, given it is completely independent from the tx signature scripts

That a suggestion from @julien-devatom. Should we?

Let's make it an npx script then, the UX will be even better. Working on it.

@Rubilmax
Copy link
Collaborator Author

Let's make it an npx script then, the UX will be even better. Working on it.

Done: https://github.com/morpho-labs/create-safe-tx

Copy link
Collaborator

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

Should we add more information about the dependency? Anyway LGTM

@Rubilmax Rubilmax merged commit 003e098 into main May 16, 2023
@Rubilmax Rubilmax deleted the feat/multisend branch May 16, 2023 15:03
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.

None yet

4 participants