Implement revokeDelegation(ByDelegator|ByProvider) #514
Conversation
…yLabs/gateway into post-monorepo-merge-411-revokeDelegation
…t-monorepo-merge-411-revokeDelegation
mattheworris
changed the title
mattheworris
changed the title
mattheworris
requested review from
JoeCap08055,
wilwade,
aramikm and
claireolmstead
apps/account-api/src/controllers/v1/delegation-v1.controller.ts
Outdated
Show resolved
Hide resolved
apps/account-api/src/controllers/v1/delegation-v1.controller.ts
Outdated
Show resolved
Hide resolved
| const { keypair } = users[1]; | ||
| const accountId = keypair.address; | ||
| const getPath: string = `/v1/delegation/revokeDelegation/${accountId}/${providerId}`; | ||
| const response = await request(app.getHttpServer()).get(getPath).expect(200); |
There was a problem hiding this comment.
Could either:
- add a separate test for
(GET) /v1/delegation/revokeDelegation/:accountId/:providerIdspecifically, -OR- - add some
expectstatements here to validate the response payload, and continue on to POST
There was a problem hiding this comment.
This seems redundant to the E2E test
There was a problem hiding this comment.
This runs against the full service and confirms that the transaction exectues, which doesn't happen for the jest based e2e tests.
There was a problem hiding this comment.
I don't see where it confirms that the transaction executes; it just calls the POST endpoint and returns at the end... I agree that's what our E2E tests should do eventually, but I think we're going to take another issue for that overhaul.
There was a problem hiding this comment.
Clarification: the transaction can be verified on the chain. No, the code doesn't check for it.
| signature: HexString, | ||
| ): Promise<[SubmittableExtrinsic<'promise'>, HexString]> { | ||
| try { | ||
| const prefixedSignature: SignerResult = { id: 1, signature }; |
There was a problem hiding this comment.
is this the same as SpRuntimeMultiSignature ?
If that is the case then one other way we created these were like the following example
const proof = { Sr25519: u8aToHex(delegatorKeys.sign(u8aWrapBytes(payload.toU8a()))) };
mattheworris
force-pushed
the
post-monorepo-merge-411-revokeDelegation
branch
from
1a14267
to
1b1919e
Compare
…t-monorepo-merge-411-revokeDelegation
mattheworris
force-pushed
the
post-monorepo-merge-411-revokeDelegation
branch
from
ed8f8e5
to
89046ac
Compare
| @@ -1,7 +1,7 @@ | |||
| // eslint-disable-next-line max-classes-per-file | |||
| import { ApiProperty } from '@nestjs/swagger'; | |||
| import { IsHexadecimal, IsNotEmpty } from 'class-validator'; | |||
| import { TransactionType } from '#account-lib'; | |||
| import { TransactionType } from '#account-lib/types/enums'; | |||
There was a problem hiding this comment.
This caused a Dependency cycle. I think this is related to the indexing problems we saw previously when importing and the services were being included when not needed.
Description
As a consumer of the Gateway API, I want to be able to revoke a previously granted delegation
Should implement:
GET /account/accounts/revokeDelegation/{accountId}/{providerId}revokeDelegationByDelegatorencoded extrinsic that can be signed)POST /account/accounts/revokeDelegationsubmits a signed extrinsic for executionCloses #411
Solution
New endpoints were added to implement the desired functionality.
Steps to Verify:
./scripts/account/restart-chain-docker.shand make sureaccount-serviceis running. You will need to additionally run the following command to start the account service, or start it locally, as the restart-chain script no longer brings up account.docker compose -f docker-compose.yaml -f docker-compose-e2e.account.yaml --profile e2e --profile account up -dcd ./apps/account-api/test/setup/ && npm run test-revoke-e2emsa.DelegationRevokedevent appears on the local node.Note: This will only work once and then the chain state needs to be restarted.