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

Allow preset overrides #4601

Merged
merged 6 commits into from Oct 7, 2022
Merged

Allow preset overrides #4601

merged 6 commits into from Oct 7, 2022

Conversation

daniildxb
Copy link
Contributor

@daniildxb daniildxb commented Sep 27, 2022

Motivation

This PR allows users to override preset values (i.e. for use in ephemerals)
Description

Adds setCustomPreset(presetName: PresetName, overrides: Partial<BeaconPreset>) method that allows users to pass preset base and override object

Closes #4598

Steps to test or reproduce
Added e2e test to params package to verify it works

@daniildxb daniildxb requested a review from a team as a code owner September 27, 2022 11:55
@CLAassistant
Copy link

CLAassistant commented Sep 27, 2022

CLA assistant check
All committers have signed the CLA.

this.timeout(30_000);

it("Should correctly override preset", async () => {
await exec(`${tsNodeBinary} ${path.join(__dirname, scriptNames.ok)}`);

Check warning

Code scanning / CodeQL

Shell command built from environment values

This shell command depends on an uncontrolled [absolute path](1). This shell command depends on an uncontrolled [absolute path](2).
});

it("Should throw trying to override preset in the wrong order", async () => {
await expect(exec(`${tsNodeBinary} ${path.join(__dirname, scriptNames.error)}`)).to.be.rejectedWith(

Check warning

Code scanning / CodeQL

Shell command built from environment values

This shell command depends on an uncontrolled [absolute path](1). This shell command depends on an uncontrolled [absolute path](2).
Copy link
Member

Choose a reason for hiding this comment

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

Can you change this test to instead be split into two different files that does the same thing (one pass, one fail)?
Wanting to prevent an exec here if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried to do it, but params/src/index.ts gets imported during test setup and freezes preset, so if I call setCustomPreset from the same thread - it fails

setCustomPreset(PresetName.minimal, {SLOTS_PER_EPOCH: 2});

// 2. Import from any other @lodestar/params paths
import {expect} from "chai";
Copy link
Contributor

Choose a reason for hiding this comment

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

Just use node:assert equals to keep the scripts simple https://nodejs.org/api/assert.html#assertequalactual-expected-message

export function setCustomPreset(presetName: PresetName, overrides: Partial<BeaconPreset>): void {
setActivePreset(presetName);
userOverrides = overrides;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not extend setActivePreset() function above?

Copy link
Member

Choose a reason for hiding this comment

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

good idea. and make overrides optional.

@wemeetagain
Copy link
Member

@daniildxb I pushed some updates here, hoping to get this merged asap

wemeetagain
wemeetagain previously approved these changes Oct 6, 2022
Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

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

this LGTM

Can someone else also give sanity check @ChainSafe/lodestar

dapplion
dapplion previously approved these changes Oct 7, 2022
Copy link
Contributor

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the contribution ❤️

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.

Allow using custom presets
4 participants