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

chore: setup caracal #413

Merged
merged 4 commits into from
Sep 22, 2023
Merged

chore: setup caracal #413

merged 4 commits into from
Sep 22, 2023

Conversation

tserg
Copy link
Collaborator

@tserg tserg commented Sep 4, 2023

Resolves #389.

@tserg tserg changed the base branch from main to migration/v2 September 4, 2023 09:02
@tserg
Copy link
Collaborator Author

tserg commented Sep 4, 2023

Marked as ready to test CI

@tserg tserg marked this pull request as ready for review September 4, 2023 09:02
.github/workflows/caracal.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@milancermak milancermak left a comment

Choose a reason for hiding this comment

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

Nice, it runs now! The reporting is a little meh, since we have to check the output every time right?

Should we be addressing the findings? There's one high impact:

 felt252-overflow Impact: High Confidence: Medium
The function aura::utils::types::StoreYangRedistribution::read uses the felt252 operation felt252_sub([62], [63]) -> ([64]), which is not overflow safe

@tserg
Copy link
Collaborator Author

tserg commented Sep 5, 2023

Nice, it runs now! The reporting is a little meh, since we have to check the output every time right?

Yes, and it looks like there can be quite a number of false positives, although there is no other alternative at the moment.

Should we be addressing the findings? There's one high impact:

 felt252-overflow Impact: High Confidence: Medium
The function aura::utils::types::StoreYangRedistribution::read uses the felt252 operation felt252_sub([62], [63]) -> ([64]), which is not overflow safe

Honestly, I am not sure what to make of this finding, whether it is a false positive and whether it is within our means to fix it. My best guess is that it relates to the corelib's implementation of the Store trait since the YangRedistribution type itself does not have a member of felt252 type.

@tserg
Copy link
Collaborator Author

tserg commented Sep 5, 2023

btw, this depends on #399.

@milancermak
Copy link
Contributor

My best guess is that it relates to the corelib's implementation of the Store trait since the YangRedistribution type itself does not have a member of felt252 type.

Seems like it, but then there's no sub operation in the derived Store impl, so not quite sure where this is coming from. Might investigate more later after the migration. For now, we can merge (once the dependencies are resolved).

@milancermak
Copy link
Contributor

Weird error. caracal detect . works for me locally

@tserg
Copy link
Collaborator Author

tserg commented Sep 8, 2023

Weird error. caracal detect . works for me locally

There were some changes to Scarb in the nightly release yesterday that broke this CI. I have filed a PR but it will need to wait for Caracal to be bumped to the next Scarb release after v0.7.0 (unless we build Caracal from source).

@milancermak
Copy link
Contributor

Good job figuring out the root cause 👏

Base automatically changed from migration/v2 to main September 15, 2023 15:38
@milancermak
Copy link
Contributor

caracal 0.2.2 was just released a couple of hours 🎉

Copy link
Contributor

@milancermak milancermak left a comment

Choose a reason for hiding this comment

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

If we're very bored we could write some parser of the caracal output to summarize it and either post it to the PR as a comment or only count the number of high impact findings and make it fail then. But I also think the tool will improve with sth similar.

@milancermak milancermak merged commit 04306f6 into main Sep 22, 2023
@milancermak milancermak deleted the chore/caracal branch September 22, 2023 08:37
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.

idea: investigate caracal
3 participants