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

feat: BACK-8055 Detect unused definitions #154

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

Conversation

ClaireGuerreGiordano
Copy link
Collaborator

This pr aims to enrich linter with unused field definition capability. If a field exists under descriptor.display.definition, it should be used elsewhere under format.

JIRA Ticket

@ledger-wiz-cspm-secret-detection
Copy link

ledger-wiz-cspm-secret-detection bot commented Nov 25, 2024

Wiz Scan Summary

Scanner Findings
Data Finding Sensitive Data 1 Info
Secret Finding Secrets
IaC Misconfiguration IaC Misconfigurations
Total 1 Info

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.

@ClaireGuerreGiordano ClaireGuerreGiordano force-pushed the fixformat branch 15 times, most recently from 36fa848 to d418e6d Compare November 26, 2024 16:17
@ClaireGuerreGiordano ClaireGuerreGiordano marked this pull request as ready for review November 26, 2024 16:25
@ClaireGuerreGiordano ClaireGuerreGiordano requested a review from a team as a code owner November 26, 2024 16:25
Copy link
Contributor

@jnicoulaud-ledger jnicoulaud-ledger 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 it makes no sense to implement this at ERC7730Linter level, since it works on resolved descriptors where we already have inlined references.

I think, the right place is to do it ERC7730InputToResolved, since this is where we use references. Just keep track of which references are used at least once in ERC7730InputToResolved, for instance by using a wrapping container that counts calls, and output a warning at the end if some have never been called.

@@ -13,5 +14,5 @@ class ERC7730Linter(ABC):
"""

@abstractmethod
def lint(self, descriptor: ResolvedERC7730Descriptor, out: OutputAdder) -> None:
def lint(self, descriptor: ResolvedERC7730Descriptor | InputERC7730Descriptor, out: OutputAdder) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

This can't be changed like this, linter runs on resolved descriptors. This only tricks the (python) linter but at runtime it takes resolved descriptors (see main class)

for future in (executor.submit(lint_file, file, input_linter, out, label(file)) for file in files):
future.result()

with ThreadPoolExecutor() as executor:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why run twice ?

resolved_descriptor = ERC7730InputToResolved().convert(input_descriptor, out)
if resolved_descriptor is not None:
linter.lint(resolved_descriptor, out)
if isinstance(linter, DefinitionLinter):
Copy link
Contributor

Choose a reason for hiding this comment

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

We can see here that it doesn't fit. Interface is not good if you have to do special cases here

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

Successfully merging this pull request may close these issues.

2 participants