Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Update Channel Terminated Message cross chain command #7696

Merged
merged 14 commits into from
Nov 7, 2022
Merged

Update Channel Terminated Message cross chain command #7696

merged 14 commits into from
Nov 7, 2022

Conversation

Phanco
Copy link
Contributor

@Phanco Phanco commented Oct 26, 2022 •
edited
Loading

What was the problem?

This PR resolves #7656

How was it solved?

Created a new BaseCCChannelTerminatedCommand inside base/cc_commands

How was it tested?

Unit Test Updated accordingly

@ishantiw ishantiw linked an issue Oct 26, 2022 that may be closed by this pull request
Update Channel Terminated Message cross chain command #7656
Closed
@Phanco Phanco marked this pull request as ready for review October 26, 2022 17:19
@Phanco
Merge branch 'feature/7211-interop-module-updates' of github.com:Lisk…
89bb62b 
…HQ/lisk-sdk into 7656-update-channel-terminated-message
ishantiw
ishantiw suggested changes Oct 28, 2022
View reviewed changes
Copy link
Contributor

@ishantiw ishantiw left a comment

Choose a reason for hiding this comment

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

maybe its better to rename, base_classes or common to be more precise and also move other base classes within it that are present within interoperability folder?

framework/src/modules/interoperability/base/cc_commands/channel_terminated.ts Outdated Show resolved Hide resolved
framework/src/modules/interoperability/schemas.ts Outdated Show resolved Hide resolved
@Phanco
Copy link
Contributor Author

Phanco commented Oct 28, 2022

maybe its better to rename, base_classes or common to be more precise and also move other base classes within it that are present within interoperability folder?

Since those classes starts with Base so I would go probably for base_classes

@Phanco
Copy link
Contributor Author

Phanco commented Oct 28, 2022

@ishantiw that involves quite a number of file changes if I move base_* files to base_classes folder. Is it okay if I do it in another PR?
image

@Phanco Phanco force-pushed the 7656-update-channel-terminated-message branch from 8fb4506 to 69a8bc9 Compare October 28, 2022 11:31
@ishantiw
Copy link
Contributor

ishantiw commented Oct 28, 2022 •
edited
Loading

@Phanco lets refactor this later in another issue,
#7696 (comment)

Meanwhile use base_cc_commands folder instead of just base and later we can rename it to base_classes and move everything else.

@Phanco Phanco force-pushed the 7656-update-channel-terminated-message branch from 778b13f to 44ece03 Compare October 28, 2022 12:52
@Phanco Phanco force-pushed the 7656-update-channel-terminated-message branch from 44ece03 to 89e571b Compare October 28, 2022 13:26
@Phanco Phanco mentioned this pull request Oct 28, 2022
Closed
@Phanco Phanco requested a review from ishantiw October 28, 2022 13:52
@sitetester
Copy link
Contributor

sitetester commented Oct 30, 2022 •
edited
Loading

@ishantiw Tests are mostly copy paste in

framework/test/unit/modules/interoperability/mainchain/cc_commands/channel_terminated.spec.ts & framework/test/unit/modules/interoperability/mainchain/cc_commands/channel_terminated.spec.ts

We can use same approach as was taken previously in case of method.spec.ts & endpoint.spec.ts

sitetester
sitetester suggested changes Oct 30, 2022
View reviewed changes
framework/test/unit/modules/interoperability/sidechain/cc_commands/channel_terminated.spec.ts Outdated Show resolved Hide resolved
framework/src/modules/interoperability/schemas.ts Outdated Show resolved Hide resolved
framework/src/modules/interoperability/base_cc_commands/channel_terminated.ts Outdated Show resolved Hide resolved
framework/src/modules/interoperability/types.ts Outdated Show resolved Hide resolved
framework/src/modules/interoperability/base_cc_commands/channel_terminated.ts Outdated Show resolved Hide resolved
framework/src/modules/interoperability/mainchain/cc_commands/channel_terminated.ts Outdated Show resolved Hide resolved
framework/src/modules/interoperability/base_cc_commands/channel_terminated.ts Outdated Show resolved Hide resolved
@Phanco Phanco force-pushed the 7656-update-channel-terminated-message branch from cbe70d1 to f13b31e Compare October 31, 2022 15:14
ishantiw
ishantiw reviewed Nov 1, 2022
View reviewed changes
Copy link
Contributor

@ishantiw ishantiw left a comment

Choose a reason for hiding this comment

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

@Phanco lgtm, maybe we can move test to one common file as @sitetester suggests. You can follow similar approach as we have in #7700

@Phanco
Copy link
Contributor Author

Phanco commented Nov 2, 2022

@Phanco lgtm, maybe we can move test to one common file as @sitetester suggests. You can follow similar approach as we have in #7700

Took reference on #7700 and updated test case, thanks :D

@shuse2 shuse2 changed the title Update Channel Terminated Message cross chain command - Closes #7656 Update Channel Terminated Message cross chain command Nov 2, 2022
@Phanco
Merge branch 'feature/7211-interop-module-updates' of github.com:Lisk…
2d83d5c 
…HQ/lisk-sdk into 7656-update-channel-terminated-message
@ishantiw ishantiw merged commit 1cf249d into feature/7211-interop-module-updates Nov 7, 2022
@ishantiw ishantiw deleted the 7656-update-channel-terminated-message branch November 7, 2022 15:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sitetester sitetester sitetester approved these changes

@ishantiw ishantiw ishantiw approved these changes

Assignees

@Phanco Phanco

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update Channel Terminated Message cross chain command
3 participants
@Phanco @ishantiw @sitetester