Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Interactive discovery #123

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Interactive discovery #123

wants to merge 14 commits into from

Conversation

sdlyy
Copy link
Member

@sdlyy sdlyy commented Jan 29, 2024

Resolves L2B-3055

What's changed

Added interactive mode where you can manage overrides (watch mode, relative, methods, etc) over the CLI.

Notes

  • comments-json used for comments preservation
  • built on top of current architecture with little to no changes made to the core

How to test it?

It will only run with some diffs present.

  1. Ideally, you have to have the main repo and tools cloned.
  2. Clone and build tools
  3. Link @l2beat/discovery - go to /packages/discovery and use yarn link in the terminal
  4. Connect the other part of the local link in the main l2beat repo via yarn link @l2beat/discovery
  5. Rebuild main repo deps - yarn install --force
  • If yarn still fails to resolve the discovery package from the tools repository (might still use remote npm under the hood), explicitly point to discovery dist as a dependency location:

image

6. Go to /package/backend/ in the main l2beat repo
7. Run yarn discover <chain> <project> --dry-run --interactive
8. Mess around
image

@sdlyy sdlyy self-assigned this Jan 29, 2024
Copy link

linear bot commented Jan 29, 2024

Copy link

changeset-bot bot commented Jan 29, 2024

⚠️ No Changeset found

Latest commit: ad9dd09

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@sdlyy sdlyy marked this pull request as ready for review January 29, 2024 18:22
"@l2beat/backend-tools": "^0.5.1",
"@l2beat/discovery-types": "^0.8.0",
"chalk": "^4.1.2",
"comment-json": "^4.2.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

We already import jsonc-parser, either don't use comment-json or replace jsonc-parser with it

@@ -70,6 +72,11 @@ export async function dryRunDiscovery(
config.chain,
)

const rawConfigWitComments = await configReader.readRawConfigWithComments(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is read here, but not used unless the interactive mode is enabled. We can just move the reading of this config into block where interactive mode is enabled.

Comment on lines +123 to +150

async readRawConfigWithComments(
name: string,
chain: string,
): Promise<RawDiscoveryConfig> {
assert(
fileExistsCaseSensitive(`discovery/${name}`),
'Project not found, check if case matches',
)
assert(
fileExistsCaseSensitive(`discovery/${name}/${chain}`),
'Chain not found in project, check if case matches',
)

const contents = await readFile(
`discovery/${name}/${chain}/config.jsonc`,
'utf-8',
)
const parsed: unknown = parseWithComments(contents)

// Parsing via Zod would effectively remove symbols and thus comments
assertDiscoveryConfig(parsed)

assert(parsed.chain === chain, 'Chain mismatch in config.jsonc')

return parsed
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There is duplication of reading the config in here now. We should extract the common part, just call readRawConfigWithComments in readConfig and drop the comments there.

constructor(private readonly dob: DiscoveryOverridesBuilder) {}

async runForDiffs(discoveryDiffs: DiscoveryDiff[]): Promise<void> {
console.log(chalk.green('Starting interactive mode...'))
Copy link
Contributor

Choose a reason for hiding this comment

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

use Logger here.

// Filter-out diffs without changes
const changesWithDiffs: Change[] = discoveryDiffs.flatMap(
(discoveryDiff) => {
const contract = discoveryDiff.name
Copy link
Contributor

Choose a reason for hiding this comment

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

The name extraction should happen only after the diff actually exists - so after the if below.

}

getContracts(): ContractParameters[] {
return [...this.output.contracts]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to spread it and later create a new array if the .contracts already is an array

* Do not replace whole file, just read most-recent raw, replace and save overrides
*/
async flushOverrides(): Promise<void> {
const path = `discovery/${this.output.name}/${this.output.chain}/config.jsonc`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we not using ConfigReader here? We implemented reading a config with comments there and just casually reimplemented this inline here

}

console.log(chalk.green('Flushing overrides...'))
await this.dob.flushOverrides()
Copy link
Contributor

Choose a reason for hiding this comment

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

Couple of things

  • we should save inside the UI
  • dob shouldn't call writeFileSync, change the name from flushOverrides to buildConfig which returns a new config with overrides that you can later save manually inside the runDiscovery

Copy link
Contributor

Choose a reason for hiding this comment

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

Can be easily tested

Copy link
Contributor

Choose a reason for hiding this comment

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

Can be easily tested

@lucadonnoh
Copy link

i'd love this <33333

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants