Skip to content
This repository has been archived by the owner on Nov 24, 2022. It is now read-only.

Provide signed messageId when removing message by Id #286

Merged
merged 2 commits into from
Sep 23, 2020

Conversation

LeonFLK
Copy link
Contributor

@LeonFLK LeonFLK commented Sep 15, 2020

fixes KILTProtocol/ticket#734

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)

Copy link
Member

@Dudleyneedham Dudleyneedham left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@tjwelde tjwelde left a comment

Choose a reason for hiding this comment

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

Looks pretty good, just a nitpick

(notification: IBlockingNotification) => {
MessageRepository.deleteByMessageId(
message.messageId as string,
selectedIdentity.identity.signStr(message.messageId as string)
Copy link
Contributor

@tjwelde tjwelde Sep 23, 2020

Choose a reason for hiding this comment

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

If messageId is a number, you can apply toString instead of using as

Copy link
Contributor Author

Choose a reason for hiding this comment

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

messageId is string | undefined
we previously returned on !messageId, now we check for existence and do a non-null assertion

@LeonFLK LeonFLK merged commit 95d3b66 into develop Sep 23, 2020
@tjwelde tjwelde deleted the lw-signed-Id-734 branch September 23, 2020 14:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants