fix(cheqd): cheqd revocationRegistryDefinition resource name #2139
Conversation
Signed-off-by: DaevMithran <[email protected]>
|
Signed-off-by: DaevMithran <[email protected]>
| @@ -308,8 +312,8 @@ export class CheqdAnonCredsRegistry implements AnonCredsRegistry { | |||
|
|
|||
| const revocationRegistryDefinitionResource = { | |||
| id: utils.uuid(), | |||
| name: credentialDefinitionName as string, | |||
| resourceType: 'anonCredsRevocRegDef', | |||
| name: `${credentialDefinitionName}-${revocationRegistryDefinition.tag}`, | |||
There was a problem hiding this comment.
we had issues with long names in the past, for which I think we came up with the solution to hash. Also, does this break already existing revocation registry definitions? Can we resolve both previously created ones, and the new one? And should the spec be updated?
There was a problem hiding this comment.
As the credential definition name is hashed already in credo will this still be a problem?
Resolution is backward compatible. we are resolving using the resource id -> get the resource name from metadata to resolve the status lists. Apologies our spec should be updated
There was a problem hiding this comment.
Okay thanks for the answers! I'm not sure, what is the max length of the tag? I think the credential definition name is max 32 because it's hashed. With the dash, the tag can max be 31 characters then. I think it should suffice, but if we really want it to be not prone to errors we should maybe re-hash?
There was a problem hiding this comment.
Both the approaches sound good to me. Let me know if I should change it to the hashed one
There was a problem hiding this comment.
Yes let's do the hash. Or the cheqd spec should mention it can max be 31 characters long, but I think that's more prone to errors down the line. So hashing seems most stable to me
packages/cheqd/package.json
Outdated
| @@ -26,7 +26,7 @@ | |||
| "test": "jest" | |||
| }, | |||
| "dependencies": { | |||
| "@cheqd/sdk": "^2.5.1", | |||
| "@cheqd/sdk": "^2.6.0", | |||
There was a problem hiding this comment.
We should properly test this in RN. We have to pin everything to a very specific version or the SDK breaks. Even with the current version it throws an unhandled promise rejection on startup (due to wasm errors) but it somehow still works for resolving.
We haven't had the time to properly look into it, but before we can merge this we should first test it in react native (actually we should pin the version to 2.5.1 for now, instead of using ^2.5.1 until it's fixed)
There was a problem hiding this comment.
Reverting it! It would be very helpful if you can create an issue on our sdk with context-rich logs or reproduction steps. Thanks
There was a problem hiding this comment.
@TimoGlastra after our mainnet upgrade we got some feedback that Cheqd transactions from credo are failing, and you can also see that some of the integration tests are failing in 2.5.1. This must be due to insufficient gas after introducing our fee market module, In our latest sdk we have fixed the default gas fees. We have not made any major dependency changes from 2.5.1 to 2.6.0
Signed-off-by: DaevMithran <[email protected]>
Signed-off-by: DaevMithran <[email protected]>
This PR fixes resource name collisions for revocation registries with different tags.
Creating two revocation registries with same name's will lead to updating the resource. Adding credential definition tag in the resource name will fix this issue