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

CommitmentTree interface updated. #74

Merged
merged 2 commits into from
Feb 4, 2022

Conversation

i-Alex
Copy link
Contributor

@i-Alex i-Alex commented Nov 25, 2021

No description provided.

api/src/lib.rs Outdated
_sc_id: jbyteArray
) -> jobject
{
let sc_id = {
Copy link
Collaborator

@DanieleDiBenedetto DanieleDiBenedetto Nov 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In these cases you can use the "deserialize_to_jobject" function to save a little bit of code lines and duplication. But don't bother, we will highly refactor the library anyway

Comment on lines +2625 to +2629
let field_class = _env.find_class("com/horizen/librustsidechains/FieldElement")
.expect("Should be able to find FieldElement class");

let initial_element = _env.new_object(field_class, "(J)V", &[
JValue::Long(0)]).expect("Should be able to create new long for FieldElement");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In these cases, you can use the "return_jobject" function to save a little bit of code lines and duplication. But don't bother, we will highly refactor the library anyway

Comment on lines +2645 to +2650
let cls_optional = _env.find_class("java/util/Optional").unwrap();

let empty_res = _env.call_static_method(cls_optional, "of", "(Ljava/lang/Object;)Ljava/util/Optional;", &[JValue::from(JObject::from(leaf_fe_array))])
.expect("Should be able to create new value for Optional");

*empty_res.l().unwrap()
Copy link
Collaborator

@DanieleDiBenedetto DanieleDiBenedetto Nov 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, not important, to save a little bit of code we might consider writing and using a generic function that wraps a JObject into another Optional JObject (or maybe directly into a jobject to return it immediately)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
I completely agree with you. In these methods there is a lot of boilerplate that should be moved to common utility methods.
Currently, just reused the same approach as we have in other places.

@DanieleDiBenedetto DanieleDiBenedetto merged commit 77de3fd into in_memory_smt Feb 4, 2022
@DanieleDiBenedetto DanieleDiBenedetto mentioned this pull request Feb 4, 2022
@DanieleDiBenedetto DanieleDiBenedetto deleted the comm_tree_update branch March 23, 2023 15:46
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.

3 participants