Issues on the current implementation

[greatect]

• With higher priority
  • Document
  • Remove the signing/verification step at the level of Transaction.Payload (VerifyAndParseDocument)
  • Authorization needs to be considered more seriously to guard security (IsDocumentAuthorized)
  • A posted document should be checked somewhere that it contains (all of) the latest descriptors that are recorded in the application. This is not considered in the current implementation, so maybe we should add IsDocumentComplete. Alternatively, we could let the application create the document automatically (generateDocument of katzenpost authority) when it calls Commit, and do not need transactions that deal with adding documents.
  • Add utility functions to help create valid documents
  • Query
  • Should provide suitable ProofOps inside the query response (for light clients)
  • Should properly define keys of the query
  • If descriptors are mentioned in the doc only through their keys, the application should also allow query for descriptors.
  • Transaction
  • Use either eddsa or ed25519 package, but not both at the same time to avoid misuse
  • Application Unit Test
  • Write tests on all types of commands (only 1 of them is covered for now)
  • The tests should use tendermint ABCIQuery to check for results with proof required
• With lower priority
  • Move syntactic and semantic checks from executeTx to isTxValid as many as possible
  • Naming on error codes inside application

[sc0Vu]

@greatect

For the validating the latest descriptors in document, we didn't log the updated/created time for the descriptors, might need to add another map for this. And what's your concern about not including the latest descriptor?

For the utility, I think we need to add another function to create descriptors too. I already write the steps to create the documents and descriptors in tests.

For the query, it look good to me. The hardest part might be fetching merkle proof from tendermint because we didn't store another proof.

I totally agree with adding more unit test and formalize the usage of eddsa/ed25519.

[greatect]

@sc0Vu

If we do not make such checks on the document, it could include a complete different set of nodes than the ones that were published on the app. So the document has to refer back to previous transactions (or the affected state) where descriptors were published.

I also mentioned the query keys because we could clarify there what type of commands we are handling then (through prefix). We could also do these taggings in our keys when updating the database. Another reason to do this is that both the mix descriptors and authorities have storing keys being their public keys (although for now they are processed with different encoding styles, but maybe we should use base64 in both), but we should allow nodes to act as both at the same time.

[sc0Vu]

With higher priority

• Document

• [x] Remove the signing/verification step at the level of Transaction.Payload (VerifyAndParseDocument)

• [ ] Authorization needs to be considered more seriously to guard security (IsDocumentAuthorized)

• [x] A posted document should be checked somewhere that it contains (all of) the latest descriptors that are recorded in the application. This is not considered in the current implementation, so maybe we should add IsDocumentComplete. Alternatively, we could let the application create the document automatically (generateDocument of katzenpost authority) when it calls Commit, and do not need transactions that deal with adding documents.

• [x] Add utility functions to help create valid documents

• Query

• [x] Should provide suitable ProofOps inside the query response (for light clients)

• [x] Should properly define keys of the query

• [x] If descriptors are mentioned in the doc only through their keys, the application should also allow query for descriptors.

• Transaction

• [x] Use either eddsa or ed25519 package, but not both at the same time to avoid misuse

• Application Unit Test

• [x] Write tests on all types of commands (only 1 of them is covered for now)

• [x] The tests should use tendermint ABCIQuery to check for results with proof required

  With lower priority

• [x] Move syntactic and semantic checks from executeTx to isTxValid as many as possible

• [x] Naming on error codes inside application

[greatect]

These issues have been dealt with