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

Detector 'Too Many Digits' is confusing, change name to 'Quantifier amount is ambiguous' #2352

Closed
sambacha opened this issue Mar 4, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@sambacha
Copy link
Contributor

sambacha commented Mar 4, 2024

Describe the desired feature

Detector 'Too Many Digits' is confusing, change name to 'Quantifier amount is ambiguous'

The Slither Detector Guide gives us the following:

contract MyContract{
    uint 1_ether = 10000000000000000000; 
}

While 1_ether looks like 1 ether, it is 10 ether. As a result, it's likely to be used incorrectly.

The name 'Too many Digits' would give you the impression that it is referring to the 10000000000000000000 and not the name 1_ether.

Proposal: Quantifier amount is ambiguous

This should be renamed to something less ambiguous, such as:

The quantifier amount does not fit with the countable noun (ether in our example).

The suggestion should read:

Consider changing the quantifier or the noun.

$value: amount → number$

Digit Spacing

Also, the name for this detector to me implies something to do with digit spacing. I would also consider the following rule:

The digits of numerical values having more than four digits on either side of the decimal marker are separated into groups of three using the underscore character _ from both the left and right of the decimal marker.

@sambacha sambacha added the enhancement New feature or request label Mar 4, 2024
@elopez
Copy link
Member

elopez commented Mar 4, 2024

Hi @sambacha, thanks for the suggestions.

For the first one, the detector currently spots constants with more than 5 consecutive zeros. Would you like to see a new detector that tries to find misleading variable names instead?

The name 'Too many Digits' would give you the impression that it is referring to the 10000000000000000000 and not the name 1_ether.

That is indeed the case; it refers to the large number constant. This other example with a different variable name triggers as well:

contract MyContract{
    uint solidity_is_fun = 10000000000000000000; 
}

@sambacha
Copy link
Contributor Author

sambacha commented Mar 5, 2024

Hi @sambacha, thanks for the suggestions.

For the first one, the detector currently spots constants with more than 5 consecutive zeros. Would you like to see a new detector that tries to find misleading variable names instead?

The name 'Too many Digits' would give you the impression that it is referring to the 10000000000000000000 and not the name 1_ether.

That is indeed the case; it refers to the large number constant. This other example with a different variable name triggers as well:

contract MyContract{

    uint solidity_is_fun = 10000000000000000000; 

}

I think actually just changing the example in the Wiki would be the best thing, as that is what confused me.

@0xalpharush
Copy link
Contributor

Would you like to make a PR?

@sambacha
Copy link
Contributor Author

sambacha commented Apr 3, 2024

Would you like to make a PR?

Seems like you beat me to it!

Quick question: can we add a registry of the different detectors that are available for slither? Is that available somewhere?

@0xalpharush
Copy link
Contributor

There is a list in the readme and the wiki. I'd like to have some site in the future but does that suffice?

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

No branches or pull requests

3 participants