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

263 add optimizations audit recommendations #392

Merged
merged 25 commits into from
Sep 8, 2023
Merged

263 add optimizations audit recommendations #392

merged 25 commits into from
Sep 8, 2023

Conversation

gianfra-t
Copy link
Contributor

@gianfra-t gianfra-t commented Sep 1, 2023 •
edited
Loading

Some notable mentions:

  • LIY-04/LIT-05 was already solved since audit.

  • LIV-01 "Duplicated Helper Function Call" -> I left this unchanged. In my opinion cannot be optimized without modifying (splitting) try_deposit_collateral from pallet vault-registry. Although in deposit_collateral extrinsic it is duplicated the active vault check, this function is used also externally outside the pallet.

  • IMP-02 "Empty Strings as prefixes" -> I also left this unchanged. I believe this could be useful in the future, since it is already coded and works, for extension purposes.

  • HOR-01/HOI-01 Unnecessary Variable cloning -> the code seems to have changed since then. Couldn't find that specific reference.

  • EXC-02/EXU-03 Double filter() calls can be reduced with filter_map() -> Is addressed along with RPC-01, where the use of closures filter and transform the into Request type inside both get_vault_redeem_requests and get_old_vault_replace_requests.

gianfra-t and others added 16 commits August 30, 2023 15:21
@gianfra-t
SYS-04 Double Iterations can be merged into a single one
3046062
@gianfra-t
SRL-02/SRW-02 limit parameter type optimization. get_latest_transacti…
3194a81 
…ons was removed so no need to change there, makes use of get_transactions
@gianfra-t
RPC-01/ Closure usage could simplify the codebase
e2c99ab
@gianfra-t
PRO-02/PRF-02 Return Type Could be an Option
640671c
@gianfra-t
LIV-01 Duplicated Helper Function Call
d05cfbc
@gianfra-t
LIP-04/LI5-04 Potential Unnecessary Computations
facd26b
@gianfra-t
LIP-03/LI5-03 Redundant Condition Check
4c5bee2
@gianfra-t
Rollback on LIV-01, try_deposit_collateral is also used in other pall…
b48b127 
…ets, cannot modify like proposed.
@gianfra-t
LII-02/LBS-01 Duplicated Condition Check
090c314
@gianfra-t
4FC-06/9B2-06 Loops optimizations with iterators
c0abe50
@gianfra-t
refactor for RPC-01
d794f37
@ebma
Fix weird indentation within macro
0cc62fa
@ebma
Replace get_old_vault_replace_requests_filter
9298605
@ebma
Fix mock type declaration issues
0f72231
@ebma
Change signature of get_vault_redeem_requests()
689e48f
@gianfra-t
RPC-01 fixed mock issue by adding constrained trait object, functions…
3c99b7b 
… are left as generic so to avoid the need of a second map and filter/transform can be done in one iterator
@gianfra-t gianfra-t requested a review from ebma September 1, 2023 19:01
@gianfra-t gianfra-t linked an issue Sep 1, 2023 that may be closed by this pull request
Add optimizations (audit recommendations) #263
Closed
13 tasks
@ebma ebma requested a review from b-yap September 4, 2023 08:39
ebma
ebma requested changes Sep 4, 2023
View reviewed changes
Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

I looked through the changes and think you did a great job @gianfra-t. I left some nitpicky comments but also one more severe issue related to changes for 'LBS-01 - Duplicated condition check'. At least I think it is, let me know your thoughts.

clients/vault/src/oracle/collector/proof_builder.rs Outdated Show resolved Hide resolved
pallets/vault-registry/src/lib.rs Outdated Show resolved Hide resolved
pallets/vault-registry/src/lib.rs Outdated Show resolved Hide resolved
primitives/src/lib.rs Outdated Show resolved Hide resolved
clients/vault/src/cancellation.rs Outdated Show resolved Hide resolved
clients/runtime/src/rpc.rs Outdated Show resolved Hide resolved
clients/runtime/src/rpc.rs Outdated Show resolved Hide resolved
@gianfra-t
Fix (potential) bug introduced before in 4FC-06/9B2-06. mut requests …
1ba4241 
…passed to drain expired() does hold active requests after execution now.
@gianfra-t
Copy link
Contributor Author

I looked through the changes and think you did a great job @gianfra-t. I left some nitpicky comments but also one more severe issue related to changes for 'LBS-01 - Duplicated condition check'. At least I think it is, let me know your thoughts.

Regarding LBS-01 (090c314), I understood that get_active_vault_from_id will match if vault if active already, if not return an error (which I mapped to the desired one in the second check). Let me know if I am missing something.

@gianfra-t
Copy link
Contributor Author

I looked through the changes and think you did a great job @gianfra-t. I left some nitpicky comments but also one more severe issue related to changes for 'LBS-01 - Duplicated condition check'. At least I think it is, let me know your thoughts.

Regarding LBS-01 (090c314), I understood that get_active_vault_from_id will match if vault if active already, if not return an error (which I mapped to the desired one in the second check). Let me know if I am missing something.

Never Mind, it is matching Active(_) not Active(true). Will rollback in that one too.

@ebma
Copy link
Member

ebma commented Sep 4, 2023

Never Mind, it is matching Active(_) not Active(true). Will rollback in that one too.

Exactly. A small but important difference. I thought I left a review comment on that specific line but maybe I accidentally discarded it again. At least I can't find it anymore haha.
Anyways, it's actually an oversight of the auditors and not on you since you just did what they asked us to do.

@gianfra-t gianfra-t requested a review from ebma September 4, 2023 22:26
ebma
ebma reviewed Sep 5, 2023
View reviewed changes
Copy link
Member

@ebma ebma left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me overall. Please install the pre-commit hook and also run cargo fmt --all to make sure that the CI does not complain (which it will because of this). You can then turn the PR from 'draft' to 'ready for review' and once CI passed, I'll approve :)

clients/vault/src/cancellation.rs Show resolved Hide resolved
@gianfra-t gianfra-t marked this pull request as ready for review September 5, 2023 14:54
@ebma
Copy link
Member

ebma commented Sep 7, 2023

@gianfra-t the CI job threw some warning because clippy failed, can you please add the suggested change and re-run the job?

ebma
ebma approved these changes Sep 8, 2023
View reviewed changes
Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

LGTM and ready for merge

@gianfra-t gianfra-t merged commit 09eabb6 into main Sep 8, 2023
@gianfra-t gianfra-t deleted the 263-add-optimizations-audit-recommendations branch September 8, 2023 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebma ebma ebma approved these changes

@b-yap b-yap Awaiting requested review from b-yap

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

Successfully merging this pull request may close these issues.

Add optimizations (audit recommendations)
2 participants
@gianfra-t @ebma