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

Fix Slither warnings and renamings #88

Merged
merged 4 commits into from
Dec 28, 2023
Merged

Fix Slither warnings and renamings #88

merged 4 commits into from
Dec 28, 2023

Conversation

PierrickGT
Copy link
Member

No description provided.

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 27, 2023 •
edited
Loading

LCOV of commit f01322d during Forge Coverage #174

Summary coverage rate:
  lines......: 88.3% (293 of 332 lines)
  functions..: 76.1% (102 of 134 functions)
  branches...: no data found

Files changed coverage rate:
                                     |Lines       |Functions  |Branches    
  Filename                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================
  src/EarnerRateModel.sol            | 100%      6| 100%     2|    -      0
  src/MToken.sol                     |97.6%     83|96.4%    28|    -      0
  src/MinterGateway.sol              | 100%    180| 100%    56|    -      0
  src/MinterRateModel.sol            | 100%      2| 100%     2|    -      0
  src/libs/TTGRegistrarReader.sol    | 0.0%     19| 0.0%    19|    -      0

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 27, 2023 •
edited
Loading

Changes to gas cost

Generated at commit: 7fcd9cb0568e7f4513db1163569e8c03cbeee20b, compared to commit: a6e0edd16b4df1fb4929fc2038a163c3b03764a3

🧾 Summary (20% most significant diffs)

Contract Method Avg (+/-) %
MToken latestIndex
latestUpdateTimestamp
rateModel		 +22 ❌
+22 ❌
+111 ❌		 +4.02%
+3.83%
+5.56%
Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
MToken 3,993,421 (+60) latestIndex
latestUpdateTimestamp
mint
rateModel
startEarning
transfer
updateIndex		 569 (+22)
597 (+22)
4,181 (0)
2,108 (+111)
26,916 (0)
43,287 (+22)
6,865 (+22)		 +4.02%
+3.83%
0.00%
+5.56%
0.00%
+0.05%
+0.32%		 569 (+22)
597 (+22)
18,916 (+6)
2,108 (+111)
29,677 (-11)
43,287 (+22)
11,396 (+22)		 +4.02%
+3.83%
+0.03%
+5.56%
-0.04%
+0.05%
+0.19%		 569 (+22)
597 (+22)
15,131 (0)
2,108 (+111)
29,677 (-11)
43,287 (+22)
12,046 (+22)		 +4.02%
+3.83%
0.00%
+5.56%
-0.04%
+0.05%
+0.18%		 569 (+22)
597 (+22)
41,222 (+22)
2,108 (+111)
32,438 (-22)
43,287 (+22)
14,363 (+22)		 +4.02%
+3.83%
+0.05%
+5.56%
-0.07%
+0.05%
+0.15%		 14 (0)
14 (0)
4 (0)
1 (0)
2 (0)
1 (0)
6 (0)

toninorair
toninorair reviewed Dec 27, 2023
View reviewed changes
@PierrickGT PierrickGT changed the title Fix Slither warnings Fix Slither warnings and renamings Dec 27, 2023
@deluca-mike deluca-mike force-pushed the feat/WEB3-586-add-EIP2098-support branch from ff15e37 to 567c44f Compare December 28, 2023 06:10
deluca-mike
deluca-mike previously requested changes Dec 28, 2023
View reviewed changes
src/Protocol.sol
@@ -510,58 +510,58 @@ contract Protocol is IProtocol, ContinuousIndexing, ERC712 {
}

/******************************************************************************************************************\
| SPOG Registrar Reader Functions |
| TTG Registrar Reader Functions |
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing anm extra space character after the title to align the right |. Although, maybe now is a good time to fix the headers with natspec/docgen-compliant ones?

Copy link
Member Author

Choose a reason for hiding this comment

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

Which header should we use? I usually use this one liner one:

    /* ============ TTG Registrar Reader Functions ============ */

Copy link
Contributor

Choose a reason for hiding this comment

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

It's nice, my only requirement is that there is a new line after it, before the start of the first line of code in that section. @toninorair Do you think a one-liner is visible enough?

Copy link
Collaborator

Choose a reason for hiding this comment

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

both look fine to me. One-liners are visible too. Let's decide on the style and address it in Jan. We can even do it even during audits.

@deluca-mike deluca-mike force-pushed the fix/WEB3-555-slither-warnings branch from e38af14 to 8f48e1b Compare December 28, 2023 07:05
@PierrickGT PierrickGT force-pushed the fix/WEB3-555-slither-warnings branch from 0b48c91 to f9db749 Compare December 28, 2023 10:00
@PierrickGT PierrickGT force-pushed the feat/WEB3-586-add-EIP2098-support branch from 567c44f to 9d7e7b3 Compare December 28, 2023 10:15
Base automatically changed from feat/WEB3-586-add-EIP2098-support to main December 28, 2023 16:35
@PierrickGT PierrickGT force-pushed the fix/WEB3-555-slither-warnings branch from f9db749 to f01322d Compare December 28, 2023 16:40
@toninorair toninorair dismissed deluca-mike’s stale review December 28, 2023 16:56

Comments addressed, change to natspec style will be discussed towards the end of pre-audit fixes

@toninorair toninorair merged commit ed292f8 into main Dec 28, 2023
@toninorair toninorair deleted the fix/WEB3-555-slither-warnings branch December 28, 2023 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toninorair toninorair toninorair approved these changes

@deluca-mike deluca-mike Awaiting requested review from deluca-mike

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

Successfully merging this pull request may close these issues.
3 participants
@PierrickGT @toninorair @deluca-mike