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

Accept credential Offer is not working. #822

Closed
ChSonendra opened this issue Jun 1, 2022 · 19 comments
Closed

Accept credential Offer is not working. #822

ChSonendra opened this issue Jun 1, 2022 · 19 comments
Assignees

Comments

@ChSonendra
Copy link

previously it was fine we just needed to pass cred_ex_id of the credential offer but now it is not working.
it expects some kind of options attribute i figured it out it needed some extra things that credential doesn't contain.
It would be great if it can be used as we used in the previous version.

@TimoGlastra
Copy link
Contributor

Yes I've also noticed this recently. This issue is already tracked in this issue: #747, but good to have a dedicated issue for it.

@NB-MikeRichardson did you already have a chance to look into this?

@NB-MikeRichardson
Copy link
Contributor

NB-MikeRichardson commented Jun 1, 2022

No i haven't looked at this issue. Will start investigating...what exactly is the issue here?

Can someone supply steps to reproduce the problem? (Just stating "but now it is not working" doesn't really help us)

@TimoGlastra
Copy link
Contributor

TimoGlastra commented Jun 1, 2022

@NB-MikeRichardson descrihed in #747:

  • accepting a proposal should take the credential definition id from the proposal to send a credential offer (if availalbe on the proposal). This functionality was there for v1 (and still is: https://github.com/hyperledger/aries-framework-javascript/blob/main/packages/core/src/modules/credentials/protocol/v1/V1CredentialService.ts#L204-L205), but itsn't present in v2. We should probably extract this from the proposal in the indy credential format service so we can reuse it across the v1 and v2 implementation. Currently throws an error that it is missing the credential definition id. It also means the user has to retrieve the message and extract the credential definition from the proposal themselves.
  • Same for the attributes, it shouldn't be required to pass the attributes when accepting a proposal as we can take them from the proposal (but should be optional for the case where there was no proposal preview and thus no attributes)

@NB-MikeRichardson
Copy link
Contributor

NB-MikeRichardson commented Jun 1, 2022

I thought the problem was with accept offer rather than accept proposal at least that's what the title of this issue says

@swcurran
Copy link
Contributor

swcurran commented Jun 1, 2022

I would think you would want to take care with taking the cred_def_id from the proposal. That data element is from the holder and may not align with what the issuer is able/willing to offer. Obvs if the cred-def-id is not one the issuer owns, they can't issue such a credential, so that check is a minimum.

@TimoGlastra
Copy link
Contributor

The method we're talking about is acceptProposal (thus indicating we want to take over the values from the proposal). If you don't want to take these values from the proposal you can either start from an offer (offerCredential) or create a counter offer (negotiateProposal)

@TimoGlastra
Copy link
Contributor

I thought the problem was with accept offer rather than accept proposal at least that's what the title of this issue says

You're right, I misread. This is probably another issue then. @ChSonendra can you add some more context on the issue? Looking at the API you should be able to call it like this:

    await this.agent.credentials.acceptOffer({
      credentialRecordId: credentialRecord.id,
    })

@ChSonendra
Copy link
Author

okay so when we have a credential offer and if we try to accept it by calling
so previously we used call this function as await agent.credentials.acceptOffer(cred_ex_id);
but in recent version that alpha-92 we have another problem this time this function is expecting another type of argument that is options "async acceptOffer(options) { function_body }" and this option argument should contain three fields that is comment, autoAcceptCredential holderDid and credential_exchange id. ideally it should work fine with cred_ex_id as it worked before. even if we pass everything we are getting this error [AriesFrameworkError: Could not determine errorName of indyError undefined].

but i really and everyone would like to accept the credential offer just by calling acceptOffer with cred_ex_id.

@NB-MikeRichardson
Copy link
Contributor

NB-MikeRichardson commented Jun 2, 2022

Looking at the code, the acceptOffer method takes one argument in the credentials module:

  public async acceptOffer(options: AcceptOfferOptions): Promise<CredentialExchangeRecord> {
}

And AcceptOfferOptions only requires a single attribute - credentialRecordId. Holder did is not a requirement for that interface.

This is the same as before so it's not clear to me where there could be a problem

@NB-MikeRichardson
Copy link
Contributor

Now I notice in that method we have some new code that was added last week to fix a bug with the Indy DID:

  const indyDid = getIndyDidFromVerficationMethod(verificationMethod)

      const requestOptions: RequestCredentialOptions = {
        comment: options.comment,
        autoAcceptCredential: options.autoAcceptCredential,
        holderDid: indyDid,
      }

      const { message, credentialRecord } = await service.createRequest(record, requestOptions)

Could there be an issue somewhere in this code?

@ChSonendra
Copy link
Author

I am debugging this piece of code and will tell the outcomes here shortly.

@ChSonendra
Copy link
Author

@NB-MikeRichardson
I passed this parameter to agent.credentials.acceptOffer() parameter

const options = {
credentialRecordId : item.id,
comment: "accept offer",
autoAcceptCredential : true
}
and this thing worked fine and state is set to
credential-received at holder and
credential-issued at issuer's end.
So the problem now is it is not completing the flow that is sending a acknowledgement after receiving the credential.

for confirmation of the issue i tried it many times even with a fresh app, but its not working.

@ChSonendra ChSonendra reopened this Jun 2, 2022
@ChSonendra
Copy link
Author

closed by mistake.

@NB-MikeRichardson
Copy link
Contributor

@ChSonendra I think the autoAcceptCredential is not a boolean but takes an enum:

export enum AutoAcceptCredential {
  // Always auto accepts the credential no matter if it changed in subsequent steps
  Always = 'always',

  // Needs one acceptation and the rest will be automated if nothing changes
  ContentApproved = 'contentApproved',

  // Never auto accept a credential
  Never = 'never',
}

Can you try that?

@ChSonendra
Copy link
Author

Thankyou @NB-MikeRichardson issue is now resolved and we have to pass

const options = {
credentialRecordId : cred_ex_id,
comment: "accept offer",
autoAcceptCredential : "always"
}

this kind of argument to acceptOffer instead of just a cred_ex_id and it works fine.
So thank You once again.

@ChSonendra ChSonendra reopened this Jun 2, 2022
@TimoGlastra
Copy link
Contributor

TimoGlastra commented Jun 2, 2022

@ChSonendra what have you set for autoAcceptCredential on the agent config? To add to this, I wouldn't recommend using always as this will mean you will accept the credential no matter the contents. We always use AutoAcceptCredential.ContentApproved which will only accept the credential if the contents aren't suddenly changed during the exchagne

@berendsliedrecht
Copy link
Contributor

@TimoGlastra Maybe we should add a warning log when AutoAcceptCredential.Always is set at on the agent config. Like

autoAcceptCredentails is set to "Always". It is not recommended for production use, and should only be used for development.

A warning might be a bit too high, so maybe an info log.

@ChSonendra
Copy link
Author

@TimoGlastra Right, So Issuer can change the content at the time of issuing of credential, thanks for mentioning this. I will try to make space to add an option for asking user to autoAcceptCredential in the App itself.

@TimoGlastra
Copy link
Contributor

This should be fixed in the latest main. @ChSonendra could you check if this is fixed now. Please reopen if the issue still persists

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

No branches or pull requests

5 participants