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.

Sign up for GitHub

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

Jump to bottom

Add SuspiciousMessageMode detector #193

Merged
merged 15 commits into from
Oct 24, 2024
Merged

Add SuspiciousMessageMode detector #193

merged 15 commits into from
Oct 24, 2024

Conversation

Esorat
Copy link
Member

@Esorat Esorat commented Oct 21, 2024 •
edited
Loading

Closes #85

  • I have updated CHANGELOG.md
  • I have added tests to demonstrate the contribution is correctly implemented
  • No test failures were reported when running yarn test-all
  • I did not do unrelated and/or undiscussed refactorings

jubnzv
jubnzv reviewed Oct 22, 2024
View reviewed changes
Copy link
Member

@jubnzv jubnzv left a comment

Choose a reason for hiding this comment

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

Great work! Let's fix the corner case and a few minor code style issues.

src/cli/cli.ts Outdated Show resolved Hide resolved
src/detectors/builtin/suspiciousMessageMode.ts Show resolved Hide resolved
src/detectors/builtin/suspiciousMessageMode.ts Outdated Show resolved Hide resolved
src/detectors/builtin/suspiciousMessageMode.ts Outdated Show resolved Hide resolved
src/detectors/builtin/suspiciousMessageMode.ts Outdated Show resolved Hide resolved
src/detectors/builtin/suspiciousMessageMode.ts Outdated Show resolved Hide resolved
@jubnzv
Copy link
Member

jubnzv commented Oct 22, 2024

We also need to update the CHANGELOG

jubnzv
jubnzv requested changes Oct 23, 2024
View reviewed changes
src/detectors/builtin/suspiciousMessageMode.ts Outdated Show resolved Hide resolved
src/detectors/builtin/suspiciousMessageMode.ts Show resolved Hide resolved
test/detectors/SuspiciousMessageMode.tact Outdated Show resolved Hide resolved
test/detectors/SuspiciousMessageMode.tact Outdated Show resolved Hide resolved
src/detectors/builtin/suspiciousMessageMode.ts Outdated Show resolved Hide resolved
src/detectors/builtin/suspiciousMessageMode.ts Outdated Show resolved Hide resolved
src/detectors/builtin/suspiciousMessageMode.ts Outdated Show resolved Hide resolved
src/detectors/builtin/suspiciousMessageMode.ts Outdated Show resolved Hide resolved
jubnzv
jubnzv reviewed Oct 23, 2024
View reviewed changes
src/detectors/builtin/suspiciousMessageMode.ts Outdated
case "method_call":
warnings.push(
this.makeWarning(
"Function calls should not be used in mode expression; use symbolic constants instead",
Copy link
Member

Choose a reason for hiding this comment

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

Why? It seems valid to me to write a function or a method that evaluates the send parameters based on some contract's state.

@jubnzv
Copy link
Member

jubnzv commented Oct 24, 2024

Let's also bump the number of detectors in README.md

@Esorat
fix: deleted unnecessary comments.
e43e6cb 
fix: Made valid to write a function or a method that evaluates the send parameters.
chore: update numbers detectors in Readme
@Esorat
fix: remake test tact contract
3ee19fb
@Esorat
Fix: refactor
6996981
jubnzv
jubnzv approved these changes Oct 24, 2024
View reviewed changes
Copy link
Member

@jubnzv jubnzv left a comment

Choose a reason for hiding this comment

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

LFG. Thank you!

src/detectors/builtin/suspiciousMessageMode.ts Outdated Show resolved Hide resolved
@jubnzv jubnzv merged commit fd9cf42 into nowarp:master Oct 24, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jubnzv jubnzv jubnzv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SendParameters Sanity Checker
2 participants
@Esorat @jubnzv