Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create substrate_client_java-milestone_2.md #530

Merged
merged 3 commits into from
Aug 24, 2022

Conversation

strategyobjectadmin
Copy link
Contributor

Milestone Delivery Checklist

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

@alxs alxs self-assigned this Aug 11, 2022
@alxs
Copy link
Contributor

alxs commented Aug 11, 2022

Thanks for the delivery @strategyobjectadmin and for the hard work, it looks like a lot of changes have gone into this milestone.

What events do you currently support, and where are you pulling the documentation from? Is this simply done manually and would it be possible to automate it, if not the generation of the classes then at least keeping the events/documentation up to date? This seems hard to maintain, although probably one can assume that events won't change all that often.

Please let me know when you've filled out the invoice form.

@strategyobjectadmin
Copy link
Contributor Author

Hi @alxs thanks for the comment. Yes, currently the events are done manually (including documentation). The goal of the first 3 milestones is to provide tooling that can allow anyone to write a definition of storage/event/extrinsic and use it. So, currently anyone have options to use predefined classes, define his own or even override predefined ones.

I agree that it might be hard to maintain. So, what we can do is to swap milestones 4 and 5. Implement support for metadata first and after that design a tool to scaffold interfaces from metadata. What do you think?

@alxs
Copy link
Contributor

alxs commented Aug 16, 2022

Makes sense, thanks for the explanation. That's a great idea! If you think that would make sense, I would fully support it. You can amend your application by simply submitting a PR to modify the original file.

Again kudos for the extensive tests. However, it would be great if you could expand on the inline documentation - not just Javadocs - in order to make your code more easily understandable to others. This is also explicitly mentioned in the deliverables and we list it as a requirement for this reason, but large parts of the project currently contain no inline documentation.

Apart from that, I tried running the balance transfer example project, but had to tweak the gradle files first. I also got several exceptions and it seemed to hang when I ran it. Could you add some instructions, perhaps also a brief description of the project and check whether it works for you? And here also, a few inline comments would probably help.

Also the documentation on events, and as I previously mentioned in general, seems a bit sparse. My impression is that there are a lot of references to other annotations, concepts etc. without context or a link to where they are defined. If you have the option, you could try getting an engineer who hasn't been involved in the project so far to provide feedback on the documentation. My Java is in pretty bad shape so maybe I also lack the intuitive understanding that comes with experience.

@strategyobjectadmin
Copy link
Contributor Author

Can you explain what was the problem with the balance transfer example please? Ideally, nothing should be modified to run it, so there might be some issue.

About the "inline documentation" I currently don't understand what that can be. Can you provide an example please? Do you mean comments in the source code? Also does it refer to the client or the the example project? The same thing about the documentation on events, do you refer to javadocs, wiki, howtos? or none of these work?

@alxs
Copy link
Contributor

alxs commented Aug 22, 2022

I had to add the following details to the inner build.gradle to be able to run the project with gradle run:

plugins {
    id 'application'
}

dependencies {
}

mainClassName = 'com.strategyobject.substrateclient.examples.balancetransfer.Main'

Maybe you were running it some other way? This is where instructions would help.

As for the error, it looks like it's just a dependency issue. This is the full output: balance-transfer.txt

Yes, by inline documentation I meant code comments, in both projects, although I think the examples are not strictly part of the grant. Happy to provide an example if it still isn't clear. As for the documentation, I meant the Wiki and HowTos.

@strategyobjectadmin
Copy link
Contributor Author

Yes, I run it differently. Anyway, I added 'application' plugin, so that it can be run with gradle. I also added a task that runs an example with 'java ...' command line, and added the instruction in the readme.

Actually by "inline documentation" we meant javadoc in the application. We generally try not use comments in the source code and encourage people to write more readable and better structured code instead.

As for the Wikis and Howtos I currently don't see how exactly it can be improved. If you have an idea, I'll be really happy to hear that. Currently, I thought the documentation will be improving when we start to receive questions. Also, I believe that when the client will be finished most of this documentation on how to manually define sections/pallets/events/... will not be needed for the endusers.

@strategyobjectadmin
Copy link
Contributor Author

Also for the error in the log. It happens because the version of glibc is less than 2.33 on you machine. It is needed for the rust lib embedded into the client. Currently, you can run the example on a newer distro (like Ubuntu 22.04), or you can compile the rust lib on your machine (https://github.com/strategyobject/substrate-client-java/tree/develop/crypto/src/jni-crypto) and set the path to the library in SUBSTRATE_CLIENT_CRYPTO_LIB_PATH env var.

This is a known issue and will be fixed here

@alxs
Copy link
Contributor

alxs commented Aug 24, 2022

Thank you for the changes and the explanations.

We generally try not use comments in the source code and encourage people to write more readable and better structured code instead.

I can't say I agree with this, but the Javadocs are the most important thing here.

In that case, feel free to expand on the external documentation later on. Mostly, I would point out what I already mentioned in my previous comment. Looking at the documentation for Web3j for example, I also noticed that it's a lot more focused on how to achieve something, rather than describing what is implemented.


Thanks, I compiled the rust lib locally and successfully ran the example project.

With this, I'm happy to tell you that the milestone is accepted. You can find my evaluation notes here. Please don't forget to fill in the invoice form and notify me about it. Also remember to amend your application if you want to swap milestones 4 and 5 or make any other changes.

@alxs alxs merged commit caf7462 into w3f:master Aug 24, 2022
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.

2 participants