feat: format test runs certification compliant with CIP-0096 ( PLT-6087)

[bogdan-manole]

Summary by CodeRabbit

• New Feature: Introduced Level 1 certification functionality, including new parsers for certification input and issuer information.
• New Feature: Added the ability to optionally include a report content ID in the Run data type.
• Refactor: Renamed various routes and functions to reflect the focus on Level 1 certifications.
• New Feature: Modified several functions to accept a DB.CertificationLevel parameter, allowing for more flexible certification processes.
• New Feature: Updated AuditorCertificationInput and introduced CertificationInput types to handle additional certification data.
• New Feature: Enhanced createCertification function to handle dry run scenarios, upload certification reports to IPFS, and create certification metadata.
• Chore: Temporarily disabled the certifyRuns function.

[coderabbitai]

Walkthrough

The changes introduce a new certification level (L1) and modify the existing certification process. The updates include new data types, modified function signatures, and updated server event selectors. The code now handles optional report content IDs, dry run scenarios, and uploads certification reports to IPFS.

Changes

File(s)	Summary
client/Main.hs	Introduced new parsers for certification input and issuer information. Replaced get-certification command with create-l1-certification.
dapps-certification-persistence/.../Run.hs	Made reportContentId field optional in the Run data type.
src/Plutus/Certification/API/Routes.hs, src/Plutus/Certification/API/Swagger.hs	Renamed CreateCertificationRoute to CreateL1CertificationRoute and removed GetCertificateRoute.
src/Plutus/Certification/Metadata.hs, src/Plutus/Certification/Metadata/Types.hs	Added new exports, commented out certLevel, and introduced CertificationInput type. Modified several function signatures to include DB.CertificationLevel.
src/Plutus/Certification/Server/Instance.hs, src/Plutus/Certification/Server/Internal.hs	Updated createCertification and getCertification functions. Introduced CreateL1CertificationField data type and updated ServerEventSelector.
src/Plutus/Certification/Synchronizer.hs	Temporarily disabled certifyRuns function.

🐇 "In the land of code, where logic is king,
A rabbit hopped forth, changes to bring.
With a skip and a hop, he tweaked the flow,
Now the certs are L1, ready to go!
Dry runs handled, reports optional too,
To IPFS they upload, as if on cue.
So here's to the rabbit, his work is done,
In the land of code, under the binary sun." 🌞

---

Tips

Chat with CodeRabbit Bot (@coderabbitai)

• Mention @coderabbitai in any review comment for bot assistance. The bot stays silent unless called upon.
• Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

• Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.

[coderabbitai]

Review Status

Actionable comments generated: 0

Commits

Files that changed from the base of the PR and between 86445ea and 7a89eee commits.

Files selected for processing (9)

• client/Main.hs (5 hunks)
• dapps-certification-persistence/src/IOHK/Certification/Persistence/Structure/Run.hs (1 hunks)
• src/Plutus/Certification/API/Routes.hs (3 hunks)
• src/Plutus/Certification/API/Swagger.hs (1 hunks)
• src/Plutus/Certification/Metadata.hs (3 hunks)
• src/Plutus/Certification/Metadata/Types.hs (2 hunks)
• src/Plutus/Certification/Server/Instance.hs (2 hunks)
• src/Plutus/Certification/Server/Internal.hs (3 hunks)
• src/Plutus/Certification/Synchronizer.hs (3 hunks)

Files skipped from review due to trivial changes (3)

• src/Plutus/Certification/API/Routes.hs
• src/Plutus/Certification/API/Swagger.hs
• src/Plutus/Certification/Synchronizer.hs

Additional comments (Suppressed): 18

dapps-certification-persistence/src/IOHK/Certification/Persistence/Structure/Run.hs (1)

• 79-79: The reportContentId field has been made optional. Ensure that this change doesn't break any existing functionality where the reportContentId was expected to be present.

src/Plutus/Certification/Server/Instance.hs (2)

• 198-250: The createCertification function has been significantly modified. It now accepts two additional parameters: certInput and dryRun. The function checks if the report content ID exists and uploads the report to IPFS if it doesn't. It also marks the run as ready for certification in the database. The function then creates the certification metadata. If dryRun is True, it just returns the metadata, otherwise it pushes the metadata to IPFS and returns the full metadata.

Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the changes do not break any existing functionality or violate any business rules or constraints. For example, check if there are any restrictions on uploading reports to IPFS or marking runs as ready for certification.

• 331-341: The createAuditorReport function now includes the certification level as a parameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the changes do not break any existing functionality or violate any business rules or constraints.

client/Main.hs (5)

• 173-248: The createCertificationParser and certificationInputParser functions have been updated to include new fields for certification input and dry run. Ensure that all calls to these functions throughout the codebase have been updated to match the new signatures.

• 260-260: The GetCertification command has been removed from the RunCommand data type. Make sure this does not break any existing functionality where this command was used.

• 267-267: The create-l1-certification command has been added to the runCommandParser function. This is a good addition as it aligns with the changes made in the PR.

• 277-277: The CreateCertificationArgs data type has been updated to include a DryRun field. This is a good change as it allows users to perform a dry run of the certification creation process.

• 670-672: The CreateCertification case in the CmdRun command now includes a dryRun parameter in the call to c.createCertification. This is a good change as it allows users to perform a dry run of the certification creation process.

src/Plutus/Certification/Metadata.hs (2)

• 81-106: The createOffchainMetadata and createOnchainMetadata functions now take an additional parameter DB.CertificationLevel. Ensure that all calls to these functions throughout the codebase have been updated to include this new parameter.

• 146-168: The createDraftMetadata and createMetadataAndPushToIpfs functions now take an additional parameter DB.CertificationLevel. Ensure that all calls to these functions throughout the codebase have been updated to include this new parameter.

src/Plutus/Certification/Server/Internal.hs (3)

• 55-59: The new CreateL1CertificationField data type introduces additional fields for the certification process. Ensure that these fields are correctly handled in all functions that use this data type.

• 96-102: The StartCertification event selector has been replaced with CreateL1Certification. Make sure that all instances of StartCertification have been updated to CreateL1Certification throughout the codebase.

• 146-150: The renderServerEventSelector function now includes cases for the new fields in CreateL1CertificationField. Check if these fields are correctly serialized to JSON.

src/Plutus/Certification/Metadata/Types.hs (5)

• 476-482: The new CertificationInput data type is introduced. It seems to be a subset of the old AuditorCertificationInput data type, excluding the report field. This change appears to be in line with the PR summary, which mentions introducing new types for certification input.

• 484-487: The AuditorCertificationInput data type has been modified to include a certificationInput field of type CertificationInput, and the report field remains. The other fields (subject, certificateIssuer, summary, disclaimer, scripts) have been moved to the CertificationInput data type. Ensure that all uses of AuditorCertificationInput throughout the codebase have been updated to reflect this change.

• 489-501: The ToJSON and FromJSON instances for AuditorCertificationInput have been updated to reflect the changes to the data type. The ToJSON instance now constructs a JSON object by merging the JSON representation of the certificationInput field with a JSON object containing the report field. The FromJSON instance parses a JSON object into an AuditorCertificationInput by parsing the entire object as a CertificationInput and optionally parsing the report field. These changes appear to be correct given the modifications to the AuditorCertificationInput data type.

• 502-508: The ToSchema instance for AuditorCertificationInput has been updated to reflect the changes to the data type. It now declares a schema for the CertificationInput field and a schema for the report field, and combines them into a named schema for AuditorCertificationInput. This change appears to be correct given the modifications to the AuditorCertificationInput data type.

• 509-516: The ToJSON, FromJSON, and ToSchema instances for CertificationInput are implemented using generic functions from the aeson and swagger2 libraries. These instances should correctly serialize and deserialize CertificationInput values to and from JSON, and declare a Swagger schema for CertificationInput, based on its data type definition.

[coderabbitai]

Review Status

Actionable comments generated: 3

Commits

Files that changed from the base of the PR and between 86445ea and ebddeb3 commits.

Files selected for processing (9)

• client/Main.hs (5 hunks)
• dapps-certification-persistence/src/IOHK/Certification/Persistence/Structure/Run.hs (1 hunks)
• src/Plutus/Certification/API/Routes.hs (3 hunks)
• src/Plutus/Certification/API/Swagger.hs (1 hunks)
• src/Plutus/Certification/Metadata.hs (3 hunks)
• src/Plutus/Certification/Metadata/Types.hs (2 hunks)
• src/Plutus/Certification/Server/Instance.hs (3 hunks)
• src/Plutus/Certification/Server/Internal.hs (3 hunks)
• src/Plutus/Certification/Synchronizer.hs (3 hunks)

Files skipped from review due to trivial changes (2)

• src/Plutus/Certification/API/Routes.hs
• src/Plutus/Certification/API/Swagger.hs

Additional comments (Suppressed): 22

dapps-certification-persistence/src/IOHK/Certification/Persistence/Structure/Run.hs (1)

• 79-79: The change made to the reportContentId field in the Run data type, making it optional (Maybe Text), is a significant one. This could potentially impact other parts of the codebase where reportContentId is used, expecting it to always have a value. Please ensure that all such instances are properly handled to avoid runtime errors.

src/Plutus/Certification/Metadata.hs (2)

• 81-106: The createOffchainMetadata and createOnchainMetadata functions now take an additional parameter DB.CertificationLevel. Ensure that all calls to these functions throughout the codebase have been updated to match the new signature.

• 146-166: The createDraftMetadata and createMetadataAndPushToIpfs functions now take an additional parameter DB.CertificationLevel. Ensure that all calls to these functions throughout the codebase have been updated to match the new signature.

client/Main.hs (5)

• 173-248: The createCertificationParser and certificationInputParser functions have been updated to include new fields for certification input and a dry run flag. Ensure that all calls to these functions throughout the codebase have been updated to match the new signature.

• 260-260: The GetCertification command has been removed from RunCommand. Make sure this does not break any existing functionality where this command was used.

• 267-267: The create-l1-certification command has been added to replace the get-certification command. Ensure that all instances of the get-certification command in the codebase have been replaced with the create-l1-certification command.

• 277-277: A new field DryRun has been added to CreateCertificationArgs. This is a good practice as it allows users to test the certification creation process without actually creating a certification.

• 670-672: The createCertification function now accepts an additional argument dryRun. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.

src/Plutus/Certification/Metadata/Types.hs (6)

• 476-482: The new CertificationInput data type is a good abstraction that encapsulates the common fields between different types of certifications. This improves code maintainability and readability.

• 484-487: The AuditorCertificationInput data type has been updated to include a CertificationInput field instead of repeating the same fields. This change adheres to the DRY (Don't Repeat Yourself) principle, improving code maintainability.

• 489-495: The custom ToJSON instance for AuditorCertificationInput ensures that the JSON object structure remains flat, even though the Haskell data structure is nested. This can improve compatibility with other systems that consume this JSON output.

• 497-500: The custom FromJSON instance for AuditorCertificationInput allows for optional "report" field in the JSON input, providing a default value of an empty list if it's not present. This makes the system more robust against missing or null values in the input.

• 502-507: The custom ToSchema instance for AuditorCertificationInput correctly combines the schemas of CertificationInput and [ReportURL]. This ensures accurate Swagger documentation for the API.

• 509-516: The use of genericToJSON, genericParseJSON, and genericDeclareNamedSchema for CertificationInput simplifies the code by leveraging GHC's DeriveGeneric extension. This reduces the chance of errors and improves maintainability.

src/Plutus/Certification/Server/Internal.hs (3)

• 52-62: The new CreateL1CertificationField data type introduces additional fields for certification input and a dry run flag. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.

• 96-102: The StartCertification case in the ServerEventSelector data type has been replaced with CreateL1Certification. Make sure that all instances of StartCertification in the codebase have been updated to CreateL1Certification.

• 146-151: The renderServerEventSelector function has been updated to handle the new CreateL1Certification case. This looks correct, but ensure that all calls to this function have been updated accordingly.

src/Plutus/Certification/Server/Instance.hs (5)

• 198-249: The createCertification function has been significantly modified. It now accepts additional parameters certInput and dryRun. The function also checks if the reportContentId exists before uploading the report to IPFS. If it doesn't exist, it uploads the report to IPFS and marks the run as ready for certification. This is a significant change from the previous implementation where the report was always uploaded to IPFS without checking for its existence. Ensure that these changes do not introduce any unintended side effects or potential data races.

• 202-202: The event type has been changed from StartCertification to CreateL1Certification. Make sure all references to this event in the codebase have been updated accordingly.

• 238-246: The function now handles dry runs differently. In case of a dry run, it just returns the metadata. Otherwise, it pushes the metadata to IPFS and returns the full metadata. Ensure that this new behavior does not conflict with other parts of the system that rely on this function.

• 331-338: The createDraftMetadata and createMetadataAndPushToIpfs functions now accept an additional parameter certLevel. Ensure that all calls to these functions throughout the codebase have been updated to match the new signature.

• 364-365: The comment indicates that L0 is used for all certificates. If this is a temporary measure, ensure that it is tracked and addressed in future updates.

[coderabbitai]

Review Status

Actionable comments generated: 5

Commits

Files that changed from the base of the PR and between 86445ea and fb21add commits.

Files selected for processing (9)

• client/Main.hs (5 hunks)
• dapps-certification-persistence/src/IOHK/Certification/Persistence/Structure/Run.hs (1 hunks)
• src/Plutus/Certification/API/Routes.hs (3 hunks)
• src/Plutus/Certification/API/Swagger.hs (1 hunks)
• src/Plutus/Certification/Metadata.hs (3 hunks)
• src/Plutus/Certification/Metadata/Types.hs (2 hunks)
• src/Plutus/Certification/Server/Instance.hs (3 hunks)
• src/Plutus/Certification/Server/Internal.hs (3 hunks)
• src/Plutus/Certification/Synchronizer.hs (3 hunks)

Files skipped from review due to trivial changes (2)

• src/Plutus/Certification/API/Routes.hs
• src/Plutus/Certification/API/Swagger.hs

Additional comments (Suppressed): 17

dapps-certification-persistence/src/IOHK/Certification/Persistence/Structure/Run.hs (1)

• 79-79: The change from a required reportContentId to an optional one (Maybe Text) is significant. Please ensure that all parts of the codebase that use this field can handle it being Nothing. Also, verify if there are any database migrations needed due to this change.

src/Plutus/Certification/Metadata/Types.hs (1)

• 28-40: No significant changes detected in this hunk. The import statements are the same as before.

client/Main.hs (5)

• 173-248: The parsers for creating certifications and their inputs are well implemented. However, there seems to be a TODO comment on line 247 about adding scripts. Please ensure this is addressed before merging the PR.

• 260-260: The GetCertification command has been removed from the RunCommand data type. Ensure that all references to this command have been updated or removed throughout the codebase.

• 267-267: The create-l1-certification command has been added to the runCommandParser. This change looks good as long as it aligns with the intended functionality of the CLI tool.

• 277-277: The CreateCertificationArgs data type now includes a DryRun field. This is a good addition as it allows users to test the certification creation process without actually executing it.

• 670-672: The CreateCertification command handler has been updated to include the dryRun argument. This change looks good as long as the createCertification function in the API client supports this argument.

src/Plutus/Certification/Server/Internal.hs (3)

• 55-60: The new CreateL1CertificationField data type introduces additional fields for certification input and a dry run flag. Ensure that these new fields are properly handled in the codebase where this data type is used.

• 96-102: The ServerEventSelector data type has been updated to replace the StartCertification case with CreateL1Certification. Make sure all instances of StartCertification have been replaced with CreateL1Certification throughout the codebase.

• 146-151: The renderServerEventSelector function has been updated to handle the new CreateL1Certification case. This function now includes handling for the new fields introduced in the CreateL1CertificationField data type. Ensure that these changes do not break existing functionality where this function is used.

src/Plutus/Certification/Synchronizer.hs (2)

• 191-196: Ensure that the removal of the certifyRuns invocation doesn't affect the functionality of the resyncWallets function and its dependencies.

• 210-216: The certifyRuns function has been commented out. If this function is crucial for the certification process, ensure that its functionality is covered elsewhere or that it will be re-enabled in the future.

src/Plutus/Certification/Server/Instance.hs (2)

• 198-239: The createCertification function has been updated to handle a new dryRun flag and certification input. The function now checks if the report content ID exists and uploads the report to IPFS if it doesn't. It also marks the run as ready for certification in the database. If it's a dry run, the function just returns the metadata; otherwise, it pushes both the metadata and the run report to IPFS and returns the full metadata. Please ensure that all calls to this function throughout the codebase have been updated to match the new signature.

• 321-328: The createDraftMetadata and createMetadataAndPushToIpfs functions are now called with an additional argument certLevel. Ensure that these functions have been updated to handle this new argument.

src/Plutus/Certification/Metadata.hs (3)

• 14-23: The import statement and the exported functions and types remain unchanged. No issues found.

• 81-109: The createOffchainMetadata function now takes two additional parameters: DB.CertificationLevel and AllowNoReport. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the logic for handling allowNoReport is correctly implemented.

- createOffchainMetadata :: (MonadIO m)
-                        => AuditorCertificationInput
-                        -> m OffChainMetadata
- createOffchainMetadata AuditorCertificationInput{..} = do
-   report' <- toReport report
-    let certificationLevel = certLevel
-    pure OffChainMetadata { report = report', ..}
+ createOffchainMetadata :: (MonadIO m)
+                        => AuditorCertificationInput
+                        -> DB.CertificationLevel
+                        -> AllowNoReport
+                        -> m OffChainMetadata
+ createOffchainMetadata AuditorCertificationInput{..} certLevel allowNoReport = do
+   let CertificationInput{..} = certificationInput
+   report' <- if allowNoReport && Prelude.null report
+                 then pure (Report [] (Hash ""))
+                 else toReport report
+    let certificationLevel = certLevel
+    pure OffChainMetadata { report = report', ..}

The createOnchainMetadata function now takes an additional parameter: DB.CertificationLevel. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.

- createOnchainMetadata :: AuditorCertificationInput
-                       -> Maybe (OffChainMetadata,[MetadataUrl])
-                       -> OnChainMetadata
- createOnchainMetadata AuditorCertificationInput{..} offchainM =
-   let (rootHash,metadata') = case offchainM of
-          Nothing -> (Hash "",[])
-          Just (offchain,metadata) ->
-            -- hash represents the encoded json of the offchain metadata
+ createOnchainMetadata :: AuditorCertificationInput
+                       -> Maybe (OffChainMetadata,[MetadataUrl])
+                       -> DB.CertificationLevel
+                       -> OnChainMetadata
+ createOnchainMetadata AuditorCertificationInput{..} offchainM certLevel =
+   let CertificationInput{..} = certificationInput
+       (rootHash,metadata') = case offchainM of
+          Nothing -> (Hash "",[])
+          Just (offchain,metadata) ->
+            -- hash represents the encoded json of the offchain metadata

[VERIFY]

• 146-173: The createDraftMetadata and createMetadataAndPushToIpfs functions now take an additional parameter: DB.CertificationLevel. Ensure that all calls to these functions throughout the codebase have been updated to match the new signatures. Also, verify that the logic for handling allowNoReport in createDraftMetadata is correctly implemented.

- createDraftMetadata :: MonadIO m
-                     => AuditorCertificationInput
-                     -> m FullMetadata
- createDraftMetadata input = do
-   offchain <- createOffchainMetadata input
-   let onchain = createOnchainMetadata input Nothing
-    pure $ FullMetadata (onchain,offchain)
+ createDraftMetadata :: MonadIO m
+                     => AuditorCertificationInput
+                     -> DB.CertificationLevel
+                     -> AllowNoReport
+                     -> m FullMetadata
+ createDraftMetadata input certLevel allowNoReport = do
+   offchain <- createOffchainMetadata input certLevel allowNoReport
+   let onchain = createOnchainMetadata input Nothing certLevel
+    pure $ FullMetadata (onchain,offchain)

- createMetadataAndPushToIpfs :: MonadIO m
-                             => AuditorCertificationInput
-                             -> m (FullMetadata,IpfsCid)
- createMetadataAndPushToIpfs input = do
-   offchain <- createOffchainMetadata input
-    finalOffchain <- addIpfsToMetadataIfNecessary offchain
-    ipfsCid <- uploadToIpfs finalOffchain
-   let onchain = createOnchainMetadata input $ Just (finalOffchain,[toMetadataUrl ipfsCid])
-    pure (FullMetadata (onchain,finalOffchain),IpfsCid ipfsCid)
+ createMetadataAndPushToIpfs :: MonadIO m
+                             => AuditorCertificationInput
+                             -> DB.CertificationLevel
+                             -> m (FullMetadata,IpfsCid)
+ createMetadataAndPushToIpfs input certLevel = do
+   offchain <- createOffchainMetadata input certLevel False
+    finalOffchain <- addIpfsToMetadataIfNecessary offchain
+    ipfsCid <- uploadToIpfs finalOffchain
+   let onchain = createOnchainMetadata input
+         (Just (finalOffchain,[toMetadataUrl ipfsCid]))
+         certLevel
+    pure (FullMetadata (onchain,finalOffchain),IpfsCid ipfsCid)

[coderabbitai]

The import statement for Plutus.Certification.Metadata is not used in the provided code snippet. If it's not used elsewhere in the file, consider removing it to keep the code clean and maintainable.

- import Plutus.Certification.Metadata

[coderabbitai]

The certifyRuns function is being exported but it's commented out in the code. If this function is not used elsewhere, consider removing this export to avoid confusion.

-   , certifyRuns

[coderabbitai]

The certLevel is currently hardcoded to DB.L0. This might limit the flexibility of your certification system. Consider making this a configurable parameter or designing a mechanism to determine the certification level dynamically based on certain conditions.

[RSoulatIOHK]

You changed the thing about L0, L1, L2, L3, the rest looks good but I haven't tried to compile it.

[RSoulatIOHK]

That's an excellent point. We need to figure it out before the EAP 😣

[RSoulatIOHK]

• Limit the certificationIssuer to 64 chars

[RSoulatIOHK]

Looks good on the code. I'll need to see through the tool usage to be sure

[amnambiar]

@bogdan-manole ,
After fixes in FE as per input-output-hk/dapps-certification-web#8 (comment),

POST https://excuse.ro/run/c65be64c-be5b-4761-b0a6-c3da4d6687fa/certificate failed with error 403 - DApp subject not available

payload -
{"certificateIssuer":{"name":"test","social":{"contact":"[email protected]","website":"test.com"}},"disclaimer":"test","scripts":[{"scriptHash":"1234567890123456789012345678901234567890123456789012345678901234","smartContractInfo":{"era":null,"compiler":null,"compilerVersion":null,"optimizer":null,"optimizerVersion":null,"progLang":null,"repoUrl":null}}],"subject":"test","summary":"test"}

Looks like CertificationInput Model doesn't have subject(mandatory) in it. This needs to be fixed either in FE or BE. Please check and update

^^ ignore

[amnambiar]

Correction to above comment -

subject need not be in payload of CertificationInput and neither be available for edit in form, as it is supposed to be taken from dapp.subject. In case of AuditorCertificationInput subject needs to be there.