feat(cred-server): generate custom credential schemas #751
Conversation
…d update references
jorgenavben
changed the title
jorgenavben
changed the title
…lt schema saving logic
…integrate it into multiple pages
…alData' for proper data handling
| @@ -223,6 +225,7 @@ const ACDC_SCHEMAS = { | |||
| description: "LE Issuer AID", | |||
| type: "string", | |||
| format: "ISO 17442", | |||
| customizable: true, | |||
| } | ||
| } | ||
|
|
||
| export default Lmdb.getInstance(); |
There was a problem hiding this comment.
If we're just going to export a single instance, we don't need to do the whole private constructor thing. You could just do
const database = new Database();
export { database }
| @@ -0,0 +1,39 @@ | |||
| import { open, RootDatabase } from "lmdb"; | |||
|
|
|||
| class Lmdb { | |||
There was a problem hiding this comment.
Usually it's better to not name these kinds of classes based on the underlying tech. So Database would be more appropriate so that you could change from lmdb to something else without many code changes across the repo.
| }); | ||
| } | ||
|
|
||
| public static getInstance(): Lmdb { |
There was a problem hiding this comment.
Not too important but no need to put public as its default. You can also use static get instance so it's more like db.instance instead of db.getInstance()
| await this.db.put(key, value); | ||
| } catch (error) { | ||
| console.error("Error putting data in LMDB:", error); | ||
| throw error; |
There was a problem hiding this comment.
There's no point catching and re-throwing if you're just logging. It's cleaner to just fully remove the try catch and let it throw, you'll get all the details.
| return res.status(404).send("Schema for given SAID not found"); | ||
| } | ||
|
|
||
| const data = schema.schema; | ||
| if (!data) { |
There was a problem hiding this comment.
Eh, we should just assume it has schema if we always store it like that. 404 implies that it's not there
| async saveSchema(schema: any, label?: string): Promise<void> { | ||
| const { saidifiedSchema, customizableKeys } = | ||
| await this.signifyApi.saidifySchema(schema, label); | ||
| let schemas = await lmdb.get(SCHEMAS_KEY); |
There was a problem hiding this comment.
better to do something like the key as schema:<schemaId> and then store them individually. Rather than a single big object for all schemas.
| if (!data) { | ||
| const schemas = await lmdb.get(SCHEMAS_KEY); | ||
| if (!schemas) { | ||
| return res.status(404).send("No schemas found"); |
There was a problem hiding this comment.
This will change based on my other comment, but yeah no need for this. Just the 404 below
There was a problem hiding this comment.
Can @sdisalvo-crd review the UI folder?
There was a problem hiding this comment.
Do you mean just the new changes or the entire folder? I've never looked into it before.
|
|
||
| async saidifySchema(schema: any, label?: string): Promise<any> { | ||
| const customizableKeys: { [key: string]: any } = {}; | ||
| const removeCustomizables = (obj: any, path: string[] = []): any => { |
There was a problem hiding this comment.
I think I need to understand this customizable part first before I understand what's happening here
There was a problem hiding this comment.
I could have flagged a lot more, but instead I just left a few comments that may help as general guidelines as I understand that I am only taking a first look at this now and in order to keep this ui work consistent with the rest of the work done in this repo we need to re-do it entirely and it's not a matter of just adding some small fix here.
| setDefaultCredentialType(""); | ||
| } | ||
| } catch (error) { | ||
| console.log(`Error fetching schema list: ${error}`); |
There was a problem hiding this comment.
I can see all errors are thrown in the console. It would be nice if we followed what we do in the app and show <ErrorMessage message={errorMessage}/> instead. Unless it's something hacky that we want to know as devs, in that case why not just throw a proper error.
| renderInput={(params) => ( | ||
| <TextField | ||
| {...params} | ||
| label="Search by connection name or ID" |
There was a problem hiding this comment.
It would be nice if we had a i18n file with all text in it.
| <Button | ||
| startIcon={<RefreshIcon />} | ||
| onClick={handleGetContacts} | ||
| style={{ height: "100%" }} |
There was a problem hiding this comment.
Inline style is old school and usually never recommended.
| xs={11} | ||
| > | ||
| <FormControl fullWidth> | ||
| <InputLabel id="credential_type">Credential Type</InputLabel> |
There was a problem hiding this comment.
It is generally recommended to use lowercase and hyphens for classes, camelCase for IDs.
| </Grid> | ||
| <Grid | ||
| item | ||
| xs={11} |
There was a problem hiding this comment.
Not sure why we are using something different (mui?) than what we already have in the app for handling styles. I would suggest to keep it consistent, instead of adopting a whole different system, however I understand this entire tool is already built so this will just be a thought.
| @@ -19,6 +19,8 @@ const NavBar: React.FC = () => { | |||
| const [anchorElNav, setAnchorElNav] = React.useState<null | HTMLElement>( | |||
| null | |||
| ); | |||
| const [anchorElCredentials, setAnchorElCredentials] = | |||
| React.useState<null | HTMLElement>(null); | |||
There was a problem hiding this comment.
You can just import React once, at the top of the file, and then avoid repeating React. every time you need to call a method.
| Credentials | ||
| </Button> | ||
| <Menu | ||
| id="credentials-menu" |
There was a problem hiding this comment.
This ID is now using dash, the one before was using an underscore. I would check that all of the IDs and class names are following the convention I mentioned before and keep it consistent.
| }) => { | ||
| useEffect(() => { | ||
| if (visible) { | ||
| const timer = setTimeout(onClose, 3000); |
There was a problem hiding this comment.
Better to assign that numeric value to a constant.
| style={{ marginBottom: "30px" }} | ||
| > | ||
| <div | ||
| style={{ | ||
| display: "flex", | ||
| gap: "10px", | ||
| marginBottom: "10px", | ||
| alignItems: "center", | ||
| }} | ||
| > | ||
| <div style={{ display: "flex", gap: "10px", flex: 1 }}> |
There was a problem hiding this comment.
Better to assign class names and add all style into a dedicated stylesheet. This apply to all inline styles in the entire folder. Please have a look around so I won't need to repeat this comment.
| <h4>Attributes</h4> | ||
| <Tooltip title="A top-level field map within an ACDC that provides a property of an entity that is inherent or assigned to the entity."> |
There was a problem hiding this comment.
Better to have al text in a i18n file.
Description
This pull request introduces new functionality for the credential issuance server, focusing on the creation and management of custom ACDC schemas. Key changes include:
Checklist before requesting a review
Issue ticket number and link
Security
Code Review