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 setup_test_client parameters #737

Merged
merged 5 commits into from
Dec 14, 2022

Conversation

Salka1988
Copy link
Member

closes #730

Now when using setup_test_client by passing both ChainConfig and ConsensusParameters, the provided parameters won't be ignored

@Salka1988 Salka1988 self-assigned this Dec 9, 2022
@Salka1988
Copy link
Member Author

Salka1988 commented Dec 9, 2022

pub async fn setup_test_client(
    coins: Vec<Coin>,
    messages: Vec<Message>,
    node_config: Option<Config>,
    chain_config: Option<ChainConfig>,
    consensus_parameters_config: Option<ConsensusParameters>,
)

regarding the second part of this PR, I can suggest that we create a builder pattern struct and put all the parameters we need in it. 🤷‍♂️
@digorithm

or we could exclude consensus_parameters because we already have them in ChainConfig and user could do something like this:

let chain_config = ChainConfig {
    transaction_parameters: ConsensusParameters::default(),
    ..ChainConfig::local_testnet()
};

@Salka1988 Salka1988 requested a review from a team December 12, 2022 10:51
@digorithm
Copy link
Member

pub async fn setup_test_client(
    coins: Vec<Coin>,
    messages: Vec<Message>,
    node_config: Option<Config>,
    chain_config: Option<ChainConfig>,
    consensus_parameters_config: Option<ConsensusParameters>,
)

regarding the second part of this PR, I can suggest that we create a builder pattern struct and put all the parameters we need in it. 🤷‍♂️ @digorithm

or we could exclude consensus_parameters because we already have them in ChainConfig and user could do something like this:

let chain_config = ChainConfig {
    transaction_parameters: ConsensusParameters::default(),
    ..ChainConfig::local_testnet()
};

That makes sense to me @Salka1988.

Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

LGTM!

@Salka1988 Salka1988 merged commit 68c5539 into master Dec 14, 2022
@Salka1988 Salka1988 deleted the Salka1988/setup_test_client_config_bug branch December 14, 2022 09:35
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.

setup_test_client uses default parameters when provided with ChainConfig and ConsensusParameters
3 participants