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

Fix multichain scripts resume functionality #6447

Merged
merged 3 commits into from
Dec 18, 2023

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Nov 28, 2023

Motivation

#4892 removed RPCs from general broadcast data and moved them to ./cache. However, for multichain sequences, RPC endpoints weren't saved anywhere which made resuming multichain scripts impossible

Solution

Save RPC endpoints used by multichain sequences to ./cache.

Ref #5047

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

I think this is reasonable

wdyt @mds1

Comment on lines 38 to 41
impl From<&mut MultiChainSequence> for SensitiveMultiChainSequence {
fn from(sequence: &mut MultiChainSequence) -> Self {
SensitiveMultiChainSequence {
deployments: sequence.deployments.iter_mut().map(|sequence| sequence.into()).collect(),
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to have this as a function instead, from impls are harder to navigate/refactor

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean something like this?

fn to_sensitive(sequence: &mut MultiChainSequence) -> SensitiveMultiChainSequence {
    SensitiveMultiChainSequence {
        deployments: sequence.deployments.iter_mut().map(|sequence| sequence.into()).collect(),
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

yeah exactly!

@mds1
Copy link
Collaborator

mds1 commented Dec 1, 2023

Haven't reviewed code carefully but agree we should fix this so multichain resume works

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

sorry, was offline a few days.

last pending nit re function, otherwise lgtm

@klkvr
Copy link
Member Author

klkvr commented Dec 8, 2023

hey @mattsse rebased and changed impl to function

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

@mattsse mattsse merged commit 6a42b0f into foundry-rs:master Dec 18, 2023
19 checks passed
@klkvr klkvr mentioned this pull request Feb 19, 2024
2 tasks
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.

4 participants