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
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/params/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ Important Notes:
- Interacting with and understanding the active preset is only necessary in very limited testing environments, eg: for ephemeral testnets
- The `minimal` preset is NOT compatible with the `mainnet` preset.
- using `setActivePreset` may be dangerous, and only should be run once before loading any other libraries. All downstream Lodestar libraries expect the active preset to never change.
- Preset values can be overriden by executing `setCustomPreset(presetName: PresetName, overrides: Partial<BeaconPreset>)` which in turn calls `setActivePreset`.
wemeetagain marked this conversation as resolved.
Show resolved Hide resolved

## License

Expand Down
4 changes: 2 additions & 2 deletions packages/params/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {preset as mainnet} from "./presets/mainnet/index.js";
import {preset as minimal} from "./presets/minimal/index.js";
import {preset as gnosis} from "./presets/gnosis/index.js";
import {presetStatus} from "./presetStatus.js";
import {userSelectedPreset} from "./setPreset.js";
import {userSelectedPreset, userOverrides} from "./setPreset.js";

export * from "./interface/index.js";
export {ForkName, ForkSeq} from "./forkName.js";
Expand Down Expand Up @@ -83,7 +83,7 @@ export const {
MAX_TRANSACTIONS_PER_PAYLOAD,
BYTES_PER_LOGS_BLOOM,
MAX_EXTRA_DATA_BYTES,
} = presets[ACTIVE_PRESET];
} = {...presets[ACTIVE_PRESET], ...userOverrides};

////////////
// Constants
Expand Down
12 changes: 12 additions & 0 deletions packages/params/src/setPreset.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {PresetName} from "./presetName.js";
import {presetStatus} from "./presetStatus.js";
import {BeaconPreset} from "./interface/index.js";

export {PresetName};

Expand Down Expand Up @@ -38,3 +39,14 @@ console.log({SLOTS_PER_EPOCH})

userSelectedPreset = presetName;
}

export let userOverrides: Partial<BeaconPreset> | null = null;

/**
* @param presetName - the preset to use as a base
* @param overrides - customized fields
*/
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.

36 changes: 36 additions & 0 deletions packages/params/test/e2e/overridePreset.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import path from "node:path";
import util from "node:util";
import child from "node:child_process";
import {fileURLToPath} from "node:url";
import {expect, use} from "chai";
import chaiAsPromised from "chai-as-promised";

const scriptNames = {
ok: "overridePresetOk.ts",
error: "overridePresetError.ts",
};

use(chaiAsPromised);

const exec = util.promisify(child.exec);

// Global variable __dirname no longer available in ES6 modules.
// Solutions: https://stackoverflow.com/questions/46745014/alternative-for-dirname-in-node-js-when-using-es6-modules
// eslint-disable-next-line @typescript-eslint/naming-convention
const __dirname = path.dirname(fileURLToPath(import.meta.url));
const tsNodeBinary = path.join(__dirname, "../../../../node_modules/.bin/ts-node-esm");

describe("Override preset", function () {
// Allow time for ts-node to compile Typescript source
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

"Lodestar preset is already frozen"
);
});
});
10 changes: 10 additions & 0 deletions packages/params/test/e2e/overridePresetError.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// This script is should be run in an e2e !!
// It demostrates how NOT to change the Lodestar preset

// 1. Import from not only @lodestar/params/setPreset will trigger an error
import {SLOTS_PER_EPOCH} from "../../lib/index.js";
import {setActivePreset, PresetName} from "../../lib/setPreset.js";
// This line should throw
setActivePreset(PresetName.minimal);

SLOTS_PER_EPOCH;
17 changes: 17 additions & 0 deletions packages/params/test/e2e/overridePresetOk.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/* eslint import/order: "off" */
// This script is should be run in an e2e !!
// It demostrates how to properly change the Lodestar preset safely

// 1. Import from @lodestar/params/setPreset only
import {setCustomPreset, PresetName} from "../../src/setPreset.js";
// eslint-disable-next-line @typescript-eslint/naming-convention
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


const {SLOTS_PER_EPOCH, BASE_REWARD_FACTOR} = await import("../../src/index.js");

expect(SLOTS_PER_EPOCH).to.equal(2, "SLOTS_PER_EPOCH should have overriden preset value");
expect(BASE_REWARD_FACTOR).to.equal(64, "BASE_REWARD_FACTOR should have preset value");
expect(process.env.LODESTAR_PRESET).to.equal(undefined, "LODESTAR_PRESET ENV must not be set");