QA Report #188
Labels
bug
Something isn't working
edited-by-warden
grade-b
Q-39
QA (Quality Assurance)
Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
QA Report low
Missing checks for address(0x0) when assigning values to address state variables
Checks are missing for all three variables below:
JBTiered721DelegateDeployer.sol: L51-53
QA Report: non-critical
Typos
JBTiered721DelegateDeployer.sol: L132
// Copy the bytecode (our initialise part is 13 bytes long)
Change
initialise
toinitialize
. The spelling of a given word should be consistent. Most of the contest uses the American English spelling ofinitialize
. For example, JBTiered721DelegateDeployer.sol: L84. The following six additional instances ofinitialise
also should be corrected:JBTiered721DelegateStore.sol: L233
JBTiered721DelegateStore.sol: L716
JBTiered721DelegateStore.sol: L947
JBTiered721DelegateStore.sol: L1032
JBTiered721DelegateStore.sol: L1183
JBBitmap.sol: L13
JBTiered721DelegateProjectDeployer.sol: L19
Change
preconfifigured
topreconfigured
JBTiered721DelegateProjectDeployer.sol: L34
Change
responsibile
toresponsible
JBTiered721Delegate.sol: L17
Delegate that offers project contributors NFTs with tiered price floors upon payment and the ability to redeem NFTs for treasury assets based based on price floor.
Remove repeated word
based
The same typo (
accross
) occurs in both lines below:JBTiered721Delegate.sol: L121
JBTiered721DelegateStore.sol: L497
@return balance The number of tokens owners by the owner accross all tiers.
Change
accross
toacross
in both casesThe same typo (
adherance
) occurs in both lines below:JBTiered721Delegate.sol: L173
JB721Delegate.sol: L176
Change
adherance
toadherence
in both casesJBTiered721Delegate.sol: L268
// Keep a reference to the number of tiers there are to mint reserved for.
Change
reserved
toreserves
The same typo (
beneificiary
) occurs in both lines below:JBTiered721Delegate.sol: L363
JBTiered721Delegate.sol: L368
Change
beneificiary
tobeneficiary
in both casesJBTiered721Delegate.sol: L545
// Keep a reference to the flag indicating if the transaction should revert if all provded funds aren't spent. Defaults to false, meaning only a minimum payment is enforced.
Change
provded
toprovided
JBTiered721Delegate.sol: L557
// Keep a reference to the the specific tier IDs to mint.
Remove repeated word
the
JBTiered721Delegate.sol: L717
Change
User
toUse
andregitered
toregistered
JBTiered721DelegateStore.sol: L176
Custom token URI resolver, superceeds base URI.
Change
superceeds
tosupersedes
JBTiered721DelegateStore.sol: L230
// Keep a referecen to the tier being iterated on.
Change
referecen
toreference
JBTiered721DelegateStore.sol: L597
@return The reserved token benficiary.
Change
benficiary
tobeneficiary
JBTiered721DelegateStore.sol: L719
// Keep a reference to the idex to iterate on next.
Change
idex
toindex
JBTiered721DelegateStore.sol: L832
// Keep a reference to the number of burned in the tier.
Change
of burned
toburned
JBTiered721DelegateStore.sol: L852
Change
reservd
toreserved
JB721Delegate.sol: L88
// Forward the recieved weight and memo, and use this contract as a pay delegate.
Change
recieved
toreceived
JB721Delegate.sol: L144
// These conditions are all part of the same curve. Edge conditions are separated because fewer operation are necessary.
Change
operation
tooperations
JBIpfsDecoder.sol: L42
Written by Martin Ludfall - Licence: MIT
Change
Licence
toLicense
NatSpec statements are missing
NatSpec statements are entirely missing for the two
constructors
referenced below:JBTiered721DelegateDeployer.sol: L46-54
JBTiered721Delegate.sol: L185-187
NatSpec statements are also entirely missing for the four
functions
below:JBTiered721DelegateDeployer.sol: L115-118
JBTiered721FundingCycleMetadataResolver.sol: L6-9
JBTiered721FundingCycleMetadataResolver.sol: L11-13
JBTiered721FundingCycleMetadataResolver.sol: L15-17
NatSpec is incomplete
The following
functions
are missing@return
:JB721TieredGovernance.sol: L68-81
JB721TieredGovernance.sol: L84-92
JB721TieredGovernance.sol: L95-108
JB721TieredGovernance.sol: L111-118
JB721TieredGovernance.sol: L121-135
JBTiered721Delegate.sol: L167-179
JB721Delegate.sol: L170-191
Long single-line comments
In theory, comments over 80 characters should wrap using multi-line comment syntax. Even if somewhat longer comments (up to 120 characters) are acceptable and a scroll bar is provided, there are cases where very long comments interfere with readability. Below are five instances of extra-long comments whose readability could be improved by wrapping, as shown:
JBTiered721DelegateDeployer.sol: L15
Suggestion:
Similarly for the following equivalent comment: JBTiered721DelegateProjectDeployer.sol: L15
JB721TieredGovernance.sol: L235
Suggestion:
JBTiered721Delegate.sol: L198
@param _pricing The tier pricing according to which token distribution will be made. Must be passed in order of contribution floor, with implied increasing value.
Suggestion:
@param _pricing The tier pricing according to which token distribution will be made. Must be passed in order of contribution floor, with implied increasing value.
JBTiered721Delegate.sol: L545
// Keep a reference to the flag indicating if the transaction should revert if all provded funds aren't spent. Defaults to false, meaning only a minimum payment is enforced.
Suggestion:
JB721Delegate.sol: L144
// These conditions are all part of the same curve. Edge conditions are separated because fewer operation are necessary.
Suggestion:
Open items
Comments that contain language referring to possible open items should be addressed and modified or removed
JBTiered721DelegateProjectDeployer.sol: L75
// Get the project ID, optimistically knowing it will be one greater than the current count.
While it might just be an example of awkward language, the phrase "optimistically knowing" appears ambiguous and should be reviewed and corrected
Use scientific notation for large multiples of 10 rather than decimal literals
For readability, used scientific notation (e.g. 1e6) rather than decimal literals (e.g. 1000000) for large multiples of ten
JBTiered721DelegateDeployer.sol: L123-124
Missing
require
revert stringAn error message should be included to enable users to understand the reason for failure.
Recommendation: Add a brief (<= 32 char) explanatory message to both of the
requires
referenced below. Or use custom error codes (which would be cheaper than revert strings).JBTiered721Delegate.sol: L216
JBTiered721Delegate.sol: L218
Use consistent initialization of counters in
for
loopsMost
for
loop counters inJuicebox
are not initiated to zero whereas a few are. It is not necessary to initializefor
loop counters to zero since this is their default value.For consistency, it makes sense to omit counter initializations in the
for
loops below.JBIpfsDecoder.sol: L49-62
JBIpfsDecoder.sol: L68-70
JBIpfsDecoder.sol: L76-78
JBIpfsDecoder.sol: L84-86
The text was updated successfully, but these errors were encountered: