Skip to content

feat: update SDK to work with refactored delegation pallet#406

Merged
ntn-x2 merged 22 commits intodevelopfrom
aa-delegation-refactor
Aug 3, 2021
Merged

feat: update SDK to work with refactored delegation pallet#406
ntn-x2 merged 22 commits intodevelopfrom
aa-delegation-refactor

Conversation

@ntn-x2
Copy link
Contributor

@ntn-x2 ntn-x2 commented Jul 28, 2021

fixes KILTProtocol/ticket#1485

Merges DelegationBaseNode and DelegationNode into DelegationNode, deletes DelegationRootNode and adds DelegationHierarchyDetails.

Integration tests are passing if using the blockchain version refactoring the delegation pallet and the latest updated types. Of course, some tests are broken as we already use the new DID pallet on the server.

One thing that requires more attention is how to synchronise the state of a node with what's on the blockchain. For instance, how to fetch the latest vector of children IDs if new nodes are added under a given node. For the moment, I have added a utility function core/src/delegation/DelegationNode.utils.ts with the signature export async function getSyncedState(delegationNode: DelegationNode): Promise<DelegationNode> to update and return a node state. We might want to think about some other ways to do that.

Checklist:

  • I have verified that the code works
  • I have verified that the code is easy to understand
  • I have left the code in a better state
  • I have documented the changes (where applicable)

@ntn-x2 ntn-x2 requested a review from rflechtner July 28, 2021 12:26
@ntn-x2 ntn-x2 marked this pull request as ready for review July 30, 2021 07:37
Copy link
Contributor

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

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

A bunch of suggestions but nothing critical. Nice work!

@ntn-x2
Copy link
Contributor Author

ntn-x2 commented Aug 2, 2021

@rflechtner good catches, thanks! I am not sure how to proceed now, as test cases are broken. Should we merge it? Should we not merge it?

@rflechtner
Copy link
Contributor

@rflechtner good catches, thanks! I am not sure how to proceed now, as test cases are broken. Should we merge it? Should we not merge it?

I say merge it - the delegationNode tests succeed and that's enough for me. Tests are broken on develop and need fixing after the DID stuff is merged.
We should however first address the issue concerning the IDelegationHierarchyDetails interface before we merge:
#406 (comment)

@ntn-x2
Copy link
Contributor Author

ntn-x2 commented Aug 3, 2021

@rflechtner please take one last look at the changes. I found a bug in the refreshState() function which did not await for the data to be fetched from the chain. I also fixed the unit and integration tests which are now working again (for delegations).

@rflechtner
Copy link
Contributor

@rflechtner please take one last look at the changes. I found a bug in the refreshState() function which did not await for the data to be fetched from the chain. I also fixed the unit and integration tests which are now working again (for delegations).

Looks good to me! Especially after the Object.assign() issue was solved

@ntn-x2 ntn-x2 merged commit 59c9d7c into develop Aug 3, 2021
@ntn-x2 ntn-x2 deleted the aa-delegation-refactor branch August 3, 2021 14:37
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