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(script): mv ScriptSequence to new crate #9098

Merged
merged 14 commits into from
Oct 15, 2024

Conversation

yash-atreya
Copy link
Member

@yash-atreya yash-atreya commented Oct 11, 2024

Motivation

Closes #9105

ScriptSequence is the type that is written to broadcast files such as run-latest.json. It handles serialization .save() and deserialization .load() of the broadcast results.

Now that we want to implement cheatcodes that make it easy to load broadcast results into a script (See: #4732). It is imperative to move these types to prevent cyclical deps.

Solution

  • Move to new crate forge-script-sequence.
  • Remove and replace the underlying type in forge-script.

@klkvr ptal

Copy link
Member

@klkvr klkvr left a comment

Choose a reason for hiding this comment

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

overall makes sense but would like us to avoid introducing duplicating types without a clear blocker for just resusing forge-script-sequence stuff

also would like us to check how many deps we'll add to foundry-cheatcodes if it would depend on new crate

crates/script/src/transaction.rs Outdated Show resolved Hide resolved
crates/script/src/sequence.rs Outdated Show resolved Hide resolved
@yash-atreya yash-atreya changed the title refactor(script): mv ScriptSequence related type to new crate refactor(script): mv ScriptSequence to new crate Oct 14, 2024
@yash-atreya yash-atreya linked an issue Oct 14, 2024 that may be closed by this pull request
@yash-atreya yash-atreya enabled auto-merge (squash) October 14, 2024 10:39
@yash-atreya
Copy link
Member Author

yash-atreya commented Oct 14, 2024

overall makes sense but would like us to avoid introducing duplicating types without a clear blocker for just resusing forge-script-sequence stuff

also would like us to check how many deps we'll add to foundry-cheatcodes if it would depend on new crate

Importing forge-script-sequence in the cheatcodes crate would add 11 more deps

Copy link
Member

@klkvr klkvr left a comment

Choose a reason for hiding this comment

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

nice, looking good, just a couple nits

crates/script-sequence/src/sequence.rs Outdated Show resolved Hide resolved
crates/script/src/transaction.rs Outdated Show resolved Hide resolved
@yash-atreya yash-atreya requested a review from klkvr October 15, 2024 08:52
@yash-atreya yash-atreya merged commit f5aa05e into master Oct 15, 2024
22 checks passed
@yash-atreya yash-atreya deleted the yash/refac-script-sequence branch October 15, 2024 11:06
rplusq pushed a commit to rplusq/foundry that referenced this pull request Nov 29, 2024
* refac(`script`): extract script sequence and related types to new crate

* replace MultiChainSequence in script crate

* replace TransactionWithMetadata and AdditionalContract

* replace ScriptSequence

* replace all underlying ScriptSequence and related types

* doc nits

* add `ScriptTransactionBuilder`

* remove `TxWithMetadata`

* mv verify fns and use `ScriptSequence` directly

* clippy
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.

refactor(script): Move ScriptSequence to new crate
2 participants