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

feat: revoke in issuer #918

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

aybarsayan
Copy link

fixes KILTProtocol/ticket#3646

Please include a summary of the changes provided with this pull request and which issue has been fixed.
Please also provide some context if necessary.

How to test:

Please provide a brief step-by-step instruction.
If necessary provide information about dependencies (specific configuration, branches, database dumps, etc.)

  • Step 1
  • Step 2
  • etc.

Checklist:

  • I have verified that the code works
  • I have verified that the code is easy to understand
    • If not, I have left a well-balanced amount of inline comments
  • I have left the code in a better state
  • I have documented the changes (where applicable)
    • Either PR or Ticket to update the Docs
    • Link the PR/Ticket here

Comment on lines 18 to 19
import * as Kilt from '@kiltprotocol/sdk-js'
import * as KiltChain from '@kiltprotocol/chain-helpers'
Copy link
Contributor

Choose a reason for hiding this comment

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

As with the other PR, no imports from sdk-js here and no blanket imports.

Copy link
Author

Choose a reason for hiding this comment

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

Done at: ccac9b9

KeyringPair,
DidUrl,
} from '@kiltprotocol/types'
import {authorizeTx} from "@kiltprotocol/did"
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure you run prettier for auto-formatting

Copy link
Author

Choose a reason for hiding this comment

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

Fixed at: ccac9b9

Comment on lines 164 to 167
attester: DidUrl,
submitterAccount: KeyringPair,
signCallback: any,
credential: VerifiableCredential,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to only see two parameters here:

Suggested change
attester: DidUrl,
submitterAccount: KeyringPair,
signCallback: any,
credential: VerifiableCredential,
credential: VerifiableCredential,
issuer: IssuerOptions,

This is enough for issuance, and will also be enough for revocation.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed at: ccac9b9

Comment on lines 169 to 206
try {
const api = Kilt.ConfigService.get('api')
const rootHash = credential.id?.split(':').pop()!
const decodedroothash = base58Decode(rootHash);

const revokeTx = api.tx.attestation.revoke(decodedroothash, null) as any
const [submitter] = (await Kilt.getSignersForKeypair({
keypair: submitterAccount,
type: 'Ed25519',
})) as Array<SignerInterface<'Ed25519', KiltAddress>>

const authorizedTx = await authorizeTx(
attester,
revokeTx,
signCallback,
submitter.id
)

const response = await KiltChain.Blockchain.signAndSubmitTx(authorizedTx, submitterAccount) as Object
const responseObj = JSON.parse(JSON.stringify(response));

return {
success: true,
info: {
blockNumber: responseObj.blockNumber,
blockHash: responseObj.status.finalized,
transactionHash: responseObj.txHash
}
}

} catch (error: unknown) {
const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred';
return {
success: false,
error: [errorMessage],
info: {}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to make sure we can support future credential- and proof types without breaking the function interfaces, this function should be set up to detect what kind of revocation status method we are dealing with, and if it's a of the KiltRevocationStatusV1 type, call the right logic to revoke the credential.

Can you move the implementation to packages/credentials/src/V1/KiltRevocationStatusV1.ts, and in the function here only check which type of credentialStatus method we are dealing with, call the function implemented in KiltRevocationStatusV1.ts if it's the right one, and error if not? the issue function above should give you an idea.

Btw all the info you need for revocation is contained in the credentialStatus id, which is composed as follows:
${chainId}/kilt:attestation/${base58Encode(rootHash)}
Because we have the chain id here as well, which contains the genesis hash of the chain on which the credential was attested, you can even check if we are connected to the right network (i.e., peregrine vs spiritnet)! See the check function in KiltRevocationStatusV1.ts for an example.

Copy link
Author

Choose a reason for hiding this comment

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

I am working on a second commit for this.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed at: 110a5cc

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