Audit fixes #1395

maltekliemann · 2024-12-02T13:25:10Z

What does it do?

What important points should reviewers know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues?

References

- Clarify `ComboMath` documentation - Make test `combo_buy_fails_on_amount_out_below_min` more precise

- Won't fix FIXME asking for better error messages - Solve FIXME in neo-swaps by adding `try_mutate_exists` to `PoolStorage`

codecov-commenter · 2024-12-05T11:38:22Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 87.21137% with 72 lines in your changes missing coverage. Please review.

Project coverage is 93.48%. Comparing base (93f0663) to head (75e5387).
Report is 2 commits behind head on develop-combo-futarchy.

Files with missing lines	Patch %	Lines
zrml/neo-swaps/src/weights.rs	66.66%	29 Missing ⚠️
...aps/src/types/decision_market_oracle_scoreboard.rs	74.13%	15 Missing ⚠️
...waps/src/types/decision_market_benchmark_helper.rs	0.00%	8 Missing ⚠️
...binatorial-tokens/src/types/collection_id_error.rs	0.00%	4 Missing ⚠️
zrml/futarchy/src/lib.rs	80.00%	4 Missing ⚠️
zrml/neo-swaps/src/types/decision_market_oracle.rs	73.33%	4 Missing ⚠️
zrml/combinatorial-tokens/src/lib.rs	89.28%	3 Missing ⚠️
zrml/futarchy/src/proposal_storage.rs	94.00%	3 Missing ⚠️
primitives/src/asset.rs	50.00%	1 Missing ⚠️
...l-tokens/src/types/cryptographic_id_manager/mod.rs	94.44%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@                    Coverage Diff                     @@
##           develop-combo-futarchy    #1395      +/-   ##
==========================================================
- Coverage                   93.53%   93.48%   -0.06%     
==========================================================
  Files                         178      183       +5     
  Lines                       34547    34931     +384     
==========================================================
+ Hits                        32315    32655     +340     
- Misses                       2232     2276      +44

Flag	Coverage Δ
tests	`93.48% <87.21%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

commit 4517504 Author: Malte Kliemann <[email protected]> Date: Tue Dec 10 18:51:24 2024 +0100 Benchmark `DecisionMarketOracle` commit 5832f0b Author: Malte Kliemann <[email protected]> Date: Tue Dec 10 16:55:48 2024 +0100 Implement `DecisionMarketOracle` using scoreboard commit e684d47 Author: Malte Kliemann <[email protected]> Date: Mon Dec 9 21:24:47 2024 +0100 Add `BlockNumber` parameter to `update` commit 5f51378 Author: Malte Kliemann <[email protected]> Date: Mon Dec 9 21:17:20 2024 +0100 Update oracles on each block commit 2b831fb Author: Malte Kliemann <[email protected]> Date: Mon Dec 9 20:40:26 2024 +0100 Implement `try_mutate_all` commit 59cb185 Author: Malte Kliemann <[email protected]> Date: Mon Dec 9 18:24:56 2024 +0100 Use `ProposalStorage` and `MaxProposals` commit 58afe87 Author: Malte Kliemann <[email protected]> Date: Mon Dec 9 17:56:53 2024 +0100 Add `MaxProposals` value commit 22068d8 Author: Malte Kliemann <[email protected]> Date: Mon Dec 9 14:15:33 2024 +0100 Add `update` function to `FutarchyOracle` trait

Chralt98 · 2025-01-14T10:44:13Z

zrml/futarchy/src/dispatchable_impls.rs

Add checks here so that the scoreboard has a reasonable configuration. For example that it is even possible to reach the victory_margin at now + duration.

Chralt98 · 2025-01-14T11:18:29Z

primitives/src/asset.rs

This isn't necessary since there is no storage value that includes the asset with index "2" (CombinatorialOutcome). For more, see here.

Chralt98 · 2025-01-14T11:21:12Z

zrml/futarchy/src/pallet_impls.rs

@@ -23,6 +23,10 @@ use frame_support::{
 };
 use zeitgeist_primitives::traits::FutarchyOracle;

+// Following Parity's implementation of pallet-democracy, we're using minimum priority for futarchy
+// proposals.
+const SCHEDULE_PRIORITY: u8 = 63;


Use HARD_DEADLINE from frame_support::traits::schedule::HARD_DEADLINE; like here.

Chralt98 · 2025-01-14T13:41:11Z

zrml/futarchy/src/lib.rs


-            let proposals = Proposals::<T>::take(now);
+            //


Suggested change

//

Chralt98 · 2025-01-14T15:23:29Z

zrml/neo-swaps/src/types/decision_market_oracle_scoreboard.rs

Especially update should be tested.

* Update arkworks-rs dependencies * Extend get_collection_id comment * Distinguish collection id generation error * Use immutable transformations * fmt * Update copyrights * Use retrieval instead of generation for collection id error * Fix misprint * fix clippy

* Add `update` function to `FutarchyOracle` trait * Add `MaxProposals` value * Use `ProposalStorage` and `MaxProposals` * Implement `try_mutate_all` * Update oracles on each block * Add `BlockNumber` parameter to `update` * Implement `DecisionMarketOracle` using scoreboard * Benchmark `DecisionMarketOracle`

maltekliemann added 2 commits December 2, 2024 14:23

(Issue No 1)

1bf57a0

- Clarify `ComboMath` documentation - Make test `combo_buy_fails_on_amount_out_below_min` more precise

Fix log_ceil and add extensive tests

965f378

mergify bot added the s:in-progress The pull requests is currently being worked on label Dec 2, 2024

maltekliemann added 10 commits December 2, 2024 21:22

(Issue No 7) Enable overflow checks

cbbdf73

(Issue No 9) Fix incorrect test

7258b52

(Issue No 10) Resolve FIXMEs

e226c42

- Won't fix FIXME asking for better error messages - Solve FIXME in neo-swaps by adding `try_mutate_exists` to `PoolStorage`

Fix formatting

bc61a30

(Issue No 13) Remove TODO by avoiding a migration

4b090cc

(Issue No 13) Remove TODO by keeping low-level types

60583ac

(Issue No 13) Remove TODO by keeping low-level types

3c9a691

(Issue No 13) Remove already fixed TODO

b899ed4

(Issue No 13) Remove won't fix TODOs

13dac24

(Issue No 13) Remove more TODOs, some by implementing CheckedIncRes

ad3089b

maltekliemann requested a review from sea212 as a code owner December 3, 2024 13:20

maltekliemann added 8 commits December 3, 2024 15:11

(Issue No 13) Remove code quality TODO

d15ab87

(Issue No 14) Define SCHEDULE_PRIORITY

1acd939

(Issue No 14) Increase readability

22ce8a2

(Issue No 14) Reuse r_over_b

499aab6

(Issue No 14) Add clarifying comments

10f67cd

(Issue No 14) Abstract common math code away

53cb4d1

Add missing file

f77898f

(Issue No 2) Replace force_max_work with fuel parameter

f664336

maltekliemann added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Dec 5, 2024

mergify bot added s:in-progress The pull requests is currently being worked on and removed s:review-needed The pull request requires reviews labels Dec 5, 2024

maltekliemann added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Dec 5, 2024

(Issue 6) Replace SubmitOrigin with root

41d8752

mergify bot added s:in-progress The pull requests is currently being worked on and removed s:review-needed The pull request requires reviews labels Dec 9, 2024

Chralt98 reviewed Jan 14, 2025

View reviewed changes

zrml/futarchy/src/lib.rs

let proposals = Proposals::<T>::take(now);

//

Copy link

Member

Chralt98 Jan 14, 2025

Choose a reason for hiding this comment

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

Suggested change

//

Chralt98 reviewed Jan 14, 2025

View reviewed changes

zrml/neo-swaps/src/types/decision_market_oracle_scoreboard.rs Outdated

Copy link

Member

Chralt98 Jan 14, 2025

Choose a reason for hiding this comment

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

Especially update should be tested.

Chralt98 and others added 2 commits January 30, 2025 14:27

Chralt98 added s:accepted This pull request is ready for merge and removed s:in-progress The pull requests is currently being worked on labels Jan 30, 2025

Chralt98 merged commit d70e33a into develop-combo-futarchy Jan 30, 2025
11 checks passed

Chralt98 deleted the mkl-audit-fixes branch January 30, 2025 13:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audit fixes #1395

Audit fixes #1395

maltekliemann commented Dec 2, 2024

codecov-commenter commented Dec 5, 2024 •

edited

Loading

Chralt98 Jan 14, 2025

Chralt98 Jan 14, 2025

Chralt98 Jan 14, 2025

Chralt98 Jan 14, 2025

Chralt98 Jan 14, 2025

Audit fixes #1395

Audit fixes #1395

Conversation

maltekliemann commented Dec 2, 2024

What does it do?

What important points should reviewers know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues?

References

codecov-commenter commented Dec 5, 2024 • edited Loading

Codecov Report

Chralt98 Jan 14, 2025

Choose a reason for hiding this comment

Chralt98 Jan 14, 2025

Choose a reason for hiding this comment

Chralt98 Jan 14, 2025

Choose a reason for hiding this comment

Chralt98 Jan 14, 2025

Choose a reason for hiding this comment

Chralt98 Jan 14, 2025

Choose a reason for hiding this comment

codecov-commenter commented Dec 5, 2024 •

edited

Loading