-
Notifications
You must be signed in to change notification settings - Fork 28
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: add support for delegation deposit reclaim #436
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor changes
@@ -64,7 +65,7 @@ export class DelegationNode implements IDelegationNode { | |||
public readonly id: IDelegationNode['id'] | |||
public readonly hierarchyId: IDelegationNode['hierarchyId'] | |||
public readonly parentId?: IDelegationNode['parentId'] | |||
public readonly childrenIds: Array<IDelegationNode['id']> = [] | |||
private childrenIdentifiers: Array<IDelegationNode['id']> = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are using id
everywhere else I would suggest to either change to match or change elsewhere to identifier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but I had to make childrenIds
a public getter, while also having a private variables, and they could not be called the same. Plus, using a leading _
would trigger lint warnings. I did not find any other way to expose what I needed to expose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do they need to be private? (also note, that after transpiling to javascript, it is not private anymore, just a hint for people working with typescript)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the idea is to follow, where possible, object-oriented encapsulation. Ok that with JS we lose all this nice info, but for TS developers it should not be possible to alter the array of children without revoking them. That is why I did not want to publicly expose it.
The tests are also failing. |
The issues with tests is because we are mistakenly referring to the [EDIT] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
childrenIds
could be kept public (just not readonly), otherwise LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Add support for delegation deposit reclaim
This PR fixes https://github.com/KILTprotocol/ticket/issues/1685 by adding a new function on
DelegationNode
and the relative chain file to generate aSubmittableExtrinsic
for thereclaim_deposit
chain function.This PR targets
develop
commit KILTprotocol/kilt-node@fc56483 andmaster
commit KILTprotocol/kilt-node@dff5ae8 (integration tests for this branch are failing as delegation deposits have not yet been merged).Companion PR (already merged on
develop
) -> KILTprotocol/kilt-node#295Checklist: