Skip to content

Review code - #1

Open
KtorZ wants to merge 9 commits into
oversize:mainfrom
KtorZ:main
Open

KtorZ wants to merge 9 commits into
oversize:mainfrom
KtorZ:main

Conversation

@KtorZ

@KtorZ KtorZ commented Oct 26, 2023

Copy link
Copy Markdown

Here's a little review with adjustments to make the code a bit more idiomatic & less redundant in places. I haven't really changed anything to the outer API, but here are a few points to consider:

  • I believe that the /metadata/{subject}/properties endpoint should return a list of strings, as per the spec. Because other implementation got this all wrong doesn't mean we should reproduce and propagate that mistake.

  • There is some inconsistency in the payloads returned by the endpoints:

    • /metadata/{subject} returns the raw object
    • /metadata/{subject}/properties/{property} returns an object { "subject": ..., property_name: ...}
    • /metadata/query returns a list of objects wrapped in a singleton { "subjects": ... }

    If this is to match existing implementations, then it's a little sad. For /metadata/query, I'd simply return the list of objects, unwrapped. For /metadata/{subject}/properties/{property}, see the next point.

  • In principle, /metadata/{subject}/properties/{property} is supposed to return a _list of objects_. Same for /metadata/{subject} actually. The rationale being that there might be multiple possible versions of the same metadata. While all metadata are signed, it is impossible to tell whether they are signed with the right key. The server can't possibly know what is "the right key" unless we start doing some KYC and have ways to connect the key to the metadata owner. This isn't the case in the cardano-token-registry coming from github because we have a 1:1 strict relationship, but servers may (should) use other mean than a github repository to store metadata. Hence, in the specification, the decision of which metadata to chose has been pushed down to the clients. Servers should therefore simply return all known metadata for that subject, with their associated signatures and let clients decide which one they want to trust.

  • The read_mappings function currently only "insert" elements into the mapping. It doesn't do any diff or remove elements that would no longer be on the file-system. This may or may not be a big problem. But perhaps something you'd like to give some thoughts?

  • The server errors are a bit opaque at the moment. It'd be nice to return some JSON object { "error": "..." } alongside the internal server errors and the not found errors.

  • You probably want to add a few more tests!

KtorZ added 9 commits October 26, 2023 12:50
  Always useful for editors that can pick it up; avoid having to manually configure things.
  Nesting the server logic under a namespace 'server' makes for nicer
  imports and tidy a bit the overall project structure instead of having
  everything in one folder.
  We should parse/sanitize inputs as soon as they're received from the user so that we fail as early as possible or can carry on with the rest of the application with some peace of mind.
  Given the potential size of the _mappings_, this isn't something we want to clone around.
  This function doesn't need to have access to the mutex (or even need
  to know that it exists). The mutext is a concern of the API layer to
  ensure concurrent access to the state. Once acquired, it suffices to
  pass a mutable reference.

  This also fixes a small 'bug' in the `read_mappings` function which
  would log the *total* entries in map rather than the new inserted
  items.

  Note also that this function doesn't get rid of any removed token (or
  blacklisted ones).
  Doesn't fail on invalid files, rather ignore them and log a warning.
  We need to acquire the lock on each API call, so we can factor-out the
  code in a nice function to that same job repeatedly. This also ensures
  that we are consistent in the server behavior and the respond send
  down to clients.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant