feat!: upgrade shared components #1606
Conversation
Signed-off-by: Ariel Gentile <[email protected]>
genaris
force-pushed
the
feat/update-shared-components
branch
from
e7841e6
to
4d4fa24
Compare
Codecov Report
@@ Coverage Diff @@
## main #1606 +/- ##
==========================================
- Coverage 85.67% 77.55% -8.12%
==========================================
Files 951 906 -45
Lines 22892 22013 -879
Branches 4024 3896 -128
==========================================
- Hits 19613 17073 -2540
- Misses 3095 4592 +1497
- Partials 184 348 +164
|
| "@types/bn.js": "^5.1.0", | ||
| "@types/ref-array-di": "^1.2.6", |
There was a problem hiding this comment.
Why do we need to add these types? Should they be added as deps to the shared components packages so we don't have to re-add them here?
There was a problem hiding this comment.
Yes, it seems that we should add them as dependency of either shared components of the patched ffi-napi/ref-napi (most likely these ones).
|
When running the test suite locally, I was getting some For example, this is a log I get: Some notes about this:
This never happened to me before (BTW I've recently upgraded to MacOS 14.0), or if it did, I fixed it permanently by running |
|
Quick update: I've been digging a little bit on this and it seems that the issue comes from anoncreds-rs, in particular the changes introduced in hyperledger/anoncreds-rs#238 in regards of calling While debugging I've added In https://docs.rs/ffi-support/latest/ffi_support/fn.destroy_c_string.html (pattern followed to define
This makes me wonder if in this case we are allocating string buffer within Node.js, meaning that it will be in charge of freeing the data when necessary (i.e. there is no actual need to call |
How is the string sent over the FFI layer? As a string, or as a reference to a string? |
|
BTW -- After thinking of it more, I'm supportive of already removing indy-sdk in 0.5.0 as well. It's causing issues and it's time it should go. If people want to keep using indy-sdk they can do so with 0.4.x. Not that it fixes the issues that are happening here. But it would help with simplifying this repo a bit |
Looking at this bit of code: public revocationRegistryDefinitionGetAttribute(options: { objectHandle: ObjectHandle; name: string }) {
const { objectHandle, name } = serializeArguments(options)
const ret = allocateStringBuffer() // IMPORTANT
this.nativeAnoncreds.anoncreds_revocation_registry_definition_get_attribute(objectHandle, name, ret)
this.handleError()
return handleReturnPointer<string>(ret, this.nativeAnoncreds.anoncreds_string_free)
}with: function handleReturnPointer<Return>(returnValue: Buffer, cleanupCallback?: (buffer: Buffer) => void): Return {
if (returnValue.address() === 0) {
throw AnoncredsError.customError({ message: 'Unexpected null pointer' })
}
const ret = returnValue.deref() as Return
if (cleanupCallback) cleanupCallback(returnValue)
return ret
}And the ErrorCode anoncreds_revocation_registry_definition_get_attribute(ObjectHandle handle,
FfiStr name,
const char **result_p);and: #[no_mangle]
pub extern "C" fn anoncreds_revocation_registry_definition_get_attribute(
handle: ObjectHandle,
name: FfiStr,
result_p: *mut *const c_char,
) -> ErrorCode {
catch_error(|| {
check_useful_c_ptr!(result_p);
let reg_def = handle.load()?;
let reg_def = reg_def.cast_ref::<RevocationRegistryDefinition>()?;
let val = match name.as_opt_str().unwrap_or_default() {
"max_cred_num" => reg_def.value.max_cred_num.to_string(),
"tails_hash" => reg_def.value.tails_hash.to_string(),
"tails_location" => reg_def.value.tails_location.to_string(),
s => return Err(err_msg!("Unsupported attribute: {}", s)),
};
unsafe { *result_p = rust_string_to_c(val) }; // IMPORTANT
Ok(())
})
}We create a |
This is the same conclusion I got now. Before, I thought this allocation in JS was just about creating a pointer, and passing it to let Rust code to actually allocate memory. Just to confirm, do you think this is the right approach, or should Node.js wrapper actually use a regular pointer so it delegates its allocation to Rust part as in others? |
Agree! I guess it will not be part of this PR but just curious about what are your thoughts regarding where to put that module. Shall we completely remove it and just mention it is available in 0.4.x in case anyone needs it? Or move it somewhere else? I'd say the first option. |
|
I say we completely remove it. If someone wants to keep using it, it would be quite easy to create a separate repo where the update the indy-sdk package to 0.5.0. I think it's important we take some steps to reduce technical debts and make it easier to maintain the repo |
I think having the pointer be managed in Node.JS is ideal right, as we then can leverage the GC from Node.JS instead of having to do it manually |
|
Created a PR: hyperledger/anoncreds-rs#253 |
|
BTW -- currently working on removing indy-sdk. But it'd be easier if this PR is merged before I open that PR (lot of conflicts otherwise) |
Signed-off-by: Ariel Gentile <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>
|
It seems this PR is stuck because it needs to run "Integration Tests (16.x)", which is not available anymore since we dropped node 16 support. It should be replaced by "Integration Tests (20.x)" I think. Does any other maintainer have permission to update this? @TimoGlastra @jakubkoci @berendsliedrecht @karimStekelenburg et al. 😄 |
|
Removed 16, added 20 |
Signed-off-by: Martin Auer <[email protected]>
Signed-off-by: Martin Auer <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>
Upgrade aries-askar, anoncreds-rs and indy-vdr to their latest versions. This implies that we are dropping node 16 support.