Skip to content

Conversation

@Romulus10
Copy link
Contributor

Milestone Delivery Checklist

Link to the application pull request: w3f/Grants-Program#873

@Romulus10
Copy link
Contributor Author

Quick bump to this PR, as well as review to make sure we did everything we needed to - thanks again!

@takahser
Copy link
Contributor

@Romulus10 thanks for your delivery and please apologize for the delay here, we currently have a rather large backlog to tackle. Nevertheless, we're going to take a look at your delivery very soon.

@Romulus10
Copy link
Contributor Author

@Romulus10 thanks for your delivery and please apologize for the delay here, we currently have a rather large backlog to tackle. Nevertheless, we're going to take a look at your delivery very soon.

Absolutely understood, no trouble at all. Just wanted to make sure we did our due diligence. Thanks again!

@dsm-w3f
Copy link
Contributor

dsm-w3f commented Sep 14, 2022

@Romulus10 thank you for the milestone submission and my apologies for the late response. Please see the evaluation document and provide proper answers and fixes. After that, let me know to continue with the evaluation.

@Romulus10
Copy link
Contributor Author

@dsm-w3f Thanks for the evaluation! We spent the week locking down on testing and documentation to fix the points you raised.

We fixed whiteflag-rust's run documentation. The scripts you mentioned were a holdover from some of our more complicated repositories.

We imitated whiteflag-java as far as testing goes, matching inputs and outputs. Any components you see that aren't directly covered in their modules' tests are covered by the higher-order tests. For example, the Message tests will cover the conditions under which encoding and cryptography are tested.

We've improved clarity and coverage of documentation - let us know if there's still anything that needs clearing up.

The final error was an issue with the crate mentioned conflicting with others - if you run our repo with its current Cargo.lock file, this error should be cleared up.

Thanks again - let us know if you run into any further issues!

@dsm-w3f
Copy link
Contributor

dsm-w3f commented Sep 26, 2022

@Romulus10 thank you for the improvements. Especially the fix of static analysis alerts was very complete.

Unfortunately, I'm still having problems running the application. Please see the updated evaluation document for details.

There are either some doubts not answered yet in the document. One way to move faster for the approval of this milestone is to perform a dry run of the tutorial on a new machine (or virtual one), especially if the responsible for this task is not involved with the development of the subject software. Sometimes we fail to catch problems in the documentation because we already used to run the software. The final user should be able to run the software without previous knowledge of it by just reading the documentation/testing guide. Let me know when the software and guides are ready for I continue with the evaluation.

@Romulus10
Copy link
Contributor Author

Romulus10 commented Sep 26, 2022

Just double-checked and have a few immediate fixes:

The reason the testing document wasn't updated for Fennel-Protocol is that it was a dependency conflict - if you use the current version of the Cargo.lock committed to the main branch, we haven't run into any further problems. That dependency was locked to a known stable version. On that note, and regarding the issue you reported under Docker, try deleting the image and rebuilding - we've so far not been able to reproduce under Docker, so it's looking likely that we're having a container version problem somewhere.

fennel-cli will run correctly through Docker now - let us know if the fix still isn't any better on your end.

fennel-app - that one was a strange one, turns out that somebody's git client or something is renaming files named 'index.js' -> 'Index.js'. I fixed it, so the most recent commit should fix the problem.

Again, please let us know if you run into any other problems. We'll keep making improvements on our end. Thanks!

@dsm-w3f
Copy link
Contributor

dsm-w3f commented Sep 27, 2022

@Romulus10 thank you for the fixes. I was able to run the application using docker for the fennel-api and runnig locally all other projects. There is some misconception in the docker containers that run tests instead of the application. I had either some problems with the web app which cannot connect to the wallet and RPC. I will also need an example for testing the application using the UI. The evaluation document was updated with the current status of the tests. Please take a look and let me know how and when I could continue.

@isaacadams
Copy link

@dsm-w3f thank you for taking the time to review our submission. Below, I have answers to your questions in the Whiteflag section.

Regarding coverage: In the file whiteflag-rust/wf_field/src/types.rs, there are some types not covered by automated tests, they were implemented and tested?

The enum MessageType holds all possible Message Types a Whiteflag message can be demarcated with. Each of these message types have a unique meaning, but they do not all have unique implementations.

Locate the definitions() function on the MessageType enum and observe that the first few types have their own unique field definitions, and then a grouping of message types share the same field definitions.

Protective, Emergency, Danger, Status, Infrastructure, Mission, and Request message types all use the same "sign" field definitions. The tests revolve more around the field definitions than around the actual message type itself. Therefore, testing one of these kinds of messages that use the "sign" field definitions is the same as testing all the message types that use the "sign" field definitions.

In the file whiteflag-rust/wf_crypto/src/encryption_method.rs, there are some encryption methods not covered by automated tests, they were tested?

In order to achieve parity with the java codebase, only AES 256 CTR was required. Any other methods present should be considered "todo". They are present because we plan to further expand the encryption algorithm support.

@dsm-w3f
Copy link
Contributor

dsm-w3f commented Sep 30, 2022

@isaacadams thank you for the answers. I checked these questions as answered in the evaluation document. I'm waiting for the other points to be solved to continue with the evaluation.

@isaacadams
Copy link

@dsm-w3f understood. @Romulus10 has been taking lead on getting the last few issues resolved. My daughter had a medical emergency this past Sunday; she is OK but still recovering. Being in the hospital most of this week, I've been limited in my ability to help bring this evaluation to a close, so that is impacting our speed.

@Romulus10
Copy link
Contributor Author

Thanks @isaacadams. This evaluation surfaced a few issues when running our frontend through Docker that we'd somehow never had an issue with before. We're making progress, but it's still not quite there. Thanks again.

@Romulus10
Copy link
Contributor Author

Hi all - we've created a new repository with a new Docker setup that fixes the issues seen in our previous deployment models. Please use

https://github.com/fennelLabs/fennel-dist

with docker compose up at the root directory to deploy a full working version of Fennel Protocol and all related software components. From there, you'll be able to point your browser at localhost:3000 and continue using the software as described in our testing guides.

@dsm-w3f
Copy link
Contributor

dsm-w3f commented Oct 5, 2022

@Romulus10 thank you for the improvements. The new repository worked well for running and testing in localhost, for use in other hosts some adjustments are necessary and they are reported in the evaluation document. We are almost done with the evaluation, the only thing that I still need to test is the CLI. I didn't find in the testing guide any example for using the CLI directly. As it is a CLI, I expected to find examples of console commands for testing. Can you please provide it? The system worked well using the web GUI.

@Romulus10
Copy link
Contributor Author

@Romulus10 thank you for the improvements. The new repository worked well for running and testing in localhost, for use in other hosts some adjustments are necessary and they are reported in the evaluation document. We are almost done with the evaluation, the only thing that I still need to test is the CLI. I didn't find in the testing guide any example for using the CLI directly. As it is a CLI, I expected to find examples of console commands for testing. Can you please provide it? The system worked well using the web GUI.

Thanks again - we'll look over those adjustments, very much appreciated.

RE: the fennel-cli, that tool is actually partially deprecated. It was intended as a development aid for the first grant, and we began to transition from a command-line-centric model to the RPC included in this milestone. We're working on documentation, but the remaining features will be transitioning over to a full RPC focus. They're testable through the encryption and Whiteflag features on fennel-app.

@dsm-w3f
Copy link
Contributor

dsm-w3f commented Oct 6, 2022

@Romulus10 In this case, since the deliverable changed, you will need to create an amendment (github PR) for this and update the original contract. After the amendment approval, I'll continue the evaluation looking the RPC instead of the CLI.

@Romulus10
Copy link
Contributor Author

fennel-cli is the tool providing the RPC as well as the formerly-used functionality, so the features we mentioned in the deliverables are present, just wrapped by the RPC and testable through fennel-app.

It's on our roadmap to rework that deprecated functionality so that the command line-specific features match the ones provided by the RPC, so we can move that to highest immediate priority and provide full testable functionality and documentation through command line arguments as well.

@isaacadams
Copy link

isaacadams commented Oct 6, 2022

@dsm-w3f thank you for your review, and we understand your concern.

I want to make it abundantly clear that the deliverable has not been changed, but rather this is a miscommunication in the language of our deliverable.

The following quote is from our deliverable, in [Milestone 2] 3. IPFS Support

The CLI and web-based frontend will be adjusted to support committing these large objects to IPFS and anchoring their location to a transaction on-chain.

This expansion will include a self-hosted IPFS node in our current Docker setup.

"The CLI" is the confusing part. We were referencing the fennel-cli repository with "The CLI"; but in hindsight, it would have been clearer to have written something more like "The RPC inside the fennel-cli repo".

The delivery that was promised was the last statement: "This expansion will include a self-hosted IPFS node in our current Docker setup."

That has been delivered.

We mentioned this would be accomplished through expanding "The CLI and our web-based frontend". Again, we should have said something more like, "Changes will be made to the fennel-cli and fennel-app repos to integrate IPFS into the RPC and web app contained within those, respectively".

We also realize that fennel-cli is a bit of a misnomer since it contains both a CLI tool and a RPC tool. They deliver nearly the same functionality, but this milestone focused on the RPC in fennel-cli, not the CLI tool in fennel-cli.

We appreciate your patience with us and apologize for our lack of specificity. We completely understand the confusion that has caused.

@dsm-w3f
Copy link
Contributor

dsm-w3f commented Oct 6, 2022

@isaacadams and @Romulus10 lets try to clarify the CLI topic. When I refer to verify the CLI is regarding the deliverable 2 of the Milestone 2.

This deliverable 2 of M2 is specified in this way in the grant proposal: "The Fennel command line client will be updated with full support for the full range of features provided by the Whiteflag Protocol. At the conclusion of the first grant, fennel-lib included encode/decode support for authentication messages and the basic set of Whiteflag message reference codes. This expansion will include all reference codes as command-line options for all message classes with some intelligent automation (e.g., if an authentication message has already been sent the interface will disallow another authenciation message from being sent until the previous is revoked), as well as full integration of fennel-lib's cryptography functions with our Whiteflag implementation.".

For me, it is clear that the deliverable 2, as stated in the grant proposal, is an extension of the CLI feature. I also could see the commands in the CLI (see the image bellow), but I don't have examples to test it. Lets check if we are talking about the same deliverable. Are the scope of deliverable 2 included in the delivery of M2? If it is, how can I test it? If not, or if it was developed in a different way (like RPC only without the command line interface) you can submit an amendment to the contract explaining the situation and we will evaluate that.

image

@Romulus10
Copy link
Contributor Author

Romulus10 commented Oct 14, 2022

Hi @dsm-w3f, we’ve got the edits we discussed wrapped up. It just took some redesign targeting arguments for the CLI rather than just the RPC we had exposed originally. Both are present, the CLI is easier to use now.

We’ve also updated our testing guide with new instructions for using all elements of the CLI. We covered Whiteflag encode/decode as well as the command to generate messages for the full range of Whiteflag message codes mentioned in our grant (cargo wf message “<code>”, where <code> is from the list in the README). IPFS support is now fully exposed through command-line arguments as well, so that covers uploading files, uploading text from an argument, removing files, and retrieving content by its CID. At this phase, that can be used for publishing Whiteflag messages or for sharing public keys by IPFS CID. Finally, we wrote out documentation for the encryption/decryption features provided by the CLI. Those were accessible through the RPC before as well, but now there’s more complete documentation.

Thanks again for your input, these evaluations are fantastic for making sure our software ends up airtight. All of these changes are on the most recent version of fennel-dist, you'll likely need to run cargo update on fennel-cli to make sure everything is running correctly.

@dsm-w3f
Copy link
Contributor

dsm-w3f commented Oct 19, 2022

@Romulus10 and @isaacadams thank you for the delivery. I tested the CLI and it worked without problems. Now the delivery is approved. Good work with the improvements.

@Romulus10
Copy link
Contributor Author

Absolutely fantastic to hear. Thanks again, we appreciate your help improving our work immensely.

@Noc2 Noc2 merged commit 9194dfb into w3f:master Oct 19, 2022
@CorruptedAesthetic
Copy link

@dsm-w3f @Noc2 @takahser
Thank you for your engagement in this engineering work. We look forward to continuing building out our technology stack, creating an MVP in the next year.

@isaacadams
Copy link

thank you @Noc2 @takahser and a special thanks to @dsm-w3f for your engagement and approval over our work!

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.

6 participants