Removing double validation #220
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes simplify the validation logic. In Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant LifecycleValidator
participant CheckLogic
Caller->>LifecycleValidator: Call validate_plugin_checks(params)
LifecycleValidator->>CheckLogic: Evaluate check_result (approve/reject)
CheckLogic-->>LifecycleValidator: Return validation status
LifecycleValidator->>Caller: Return approval or rejection
sequenceDiagram
participant Requester
participant AssetValidator
participant PluginValidator
Requester->>AssetValidator: Call validate_asset_permissions(assetKey)
AssetValidator->>PluginValidator: Validate using AssetV1 logic
PluginValidator-->>AssetValidator: Return result
AssetValidator->>Requester: Return permission status
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
PR Overview
This PR removes redundant validations by eliminating separate key-based checks for Asset and Collection plugins, as deduplication is already managed by the checks BTreeMap.
- Removed the key parameter and associated equality checks in both plugin validation functions.
- Updated calls in the utils module to reflect the new function signatures and consolidated validation logic.
Reviewed Changes
| File | Description |
|---|---|
| programs/mpl-core/src/plugins/lifecycle.rs | Removed key comparisons from validate_plugin_checks and validate_external_plugin_adapter_checks to avoid double validation. |
| programs/mpl-core/src/utils/mod.rs | Updated invocations of plugin validation functions to remove the now-unused key parameter. |
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
programs/mpl-core/src/plugins/lifecycle.rs:685
- Ensure that removing the key equality check in validate_plugin_checks does not allow validations for unintended plugin types. Confirm that deduplication using the checks BTreeMap fully prevents processing unrelated checks.
if matches!(check_result, CheckResult::CanApprove | CheckResult::CanReject) {
programs/mpl-core/src/plugins/lifecycle.rs:761
- Verify that removing the key comparison in validate_external_plugin_adapter_checks does not inadvertently validate checks for unrelated keys. Rely solely on the check_result flags with confidence that the checks map is managed appropriately.
if check_result.can_listen() || check_result.can_approve() || check_result.can_reject() {
programs/mpl-core/src/utils/mod.rs:243
- [nitpick] Consider adding or updating tests to verify that the new function signatures and consolidated validation logic behave as expected for both asset and collection validations.
match validate_plugin_checks(
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Benchmark
| Benchmark suite | Current: f72daf1 | Previous: 661d5e0 | Ratio |
|---|---|---|---|
CU: create a new, empty asset |
11095 Compute Units |
11206 Compute Units |
0.99 |
Space: create a new, empty asset |
91 Bytes |
91 Bytes |
1 |
CU: create a new, empty asset with empty collection |
22570 Compute Units |
22681 Compute Units |
1.00 |
Space: create a new, empty asset with empty collection |
91 Bytes |
91 Bytes |
1 |
CU: create a new asset with plugins |
35071 Compute Units |
35182 Compute Units |
1.00 |
Space: create a new asset with plugins |
194 Bytes |
194 Bytes |
1 |
CU: create a new asset with plugins and empty collection |
41091 Compute Units |
41202 Compute Units |
1.00 |
Space: create a new asset with plugins and empty collection |
194 Bytes |
194 Bytes |
1 |
CU: list an asset |
27903 Compute Units |
28101 Compute Units |
0.99 |
CU: sell an asset |
43944 Compute Units |
44473 Compute Units |
0.99 |
CU: list an asset with empty collection |
35467 Compute Units |
35663 Compute Units |
0.99 |
CU: sell an asset with empty collection |
56299 Compute Units |
56825 Compute Units |
0.99 |
CU: list an asset with collection royalties |
36116 Compute Units |
36434 Compute Units |
0.99 |
CU: sell an asset with collection royalties |
61287 Compute Units |
61909 Compute Units |
0.99 |
CU: transfer an empty asset |
6333 Compute Units |
6444 Compute Units |
0.98 |
CU: transfer an empty asset with empty collection |
8933 Compute Units |
9043 Compute Units |
0.99 |
CU: transfer an asset with plugins |
14472 Compute Units |
14769 Compute Units |
0.98 |
CU: transfer an asset with plugins and empty collection |
17072 Compute Units |
17368 Compute Units |
0.98 |
This comment was automatically generated by workflow using github-action-benchmark.
After reviewing the asset validation code it seems like we're unnecessarily performing separate validations on both Asset and Collection plugins. Deduplication is already handled by the
checksBtreeMapso I don't believe we actually need to do these separatelyIf that's not the case then we need to update the tests to appropriately prove out why these validations should be separate.
Summary by CodeRabbit