Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Create Group implementation #20

Merged
merged 1 commit into from
Mar 3, 2020
Merged

Create Group implementation #20

merged 1 commit into from
Mar 3, 2020

Conversation

alpe
Copy link
Collaborator

@alpe alpe commented Feb 17, 2020 •
edited
Loading

Implement basic workflow to create a group

  • Adds Keeper, handler, module, integration test setup

Happy path mostly. Proper implementation and cleaning up after #24

@alpe alpe requested a review from aaronc February 17, 2020 16:09
aaronc
aaronc suggested changes Feb 17, 2020
View reviewed changes
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

It seems that there are several commits in here that should already be in module/group. Is there something off with that branch or is it this branch?

incubator/group/codec.go Show resolved Hide resolved
incubator/group/group.go
}

func (a AppModule) RegisterCodec(cdc *codec.Codec) {
RegisterCodec(cdc)
Copy link
Member

Choose a reason for hiding this comment

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

We can just leave this blank. We don't need amino.

incubator/group/group.go Outdated Show resolved Hide resolved
incubator/group/handler.go Show resolved Hide resolved
incubator/group/keeper.go Show resolved Hide resolved
incubator/group/msg.go Show resolved Hide resolved
incubator/group/msg.go Outdated Show resolved Hide resolved
incubator/group/types.go Outdated Show resolved Hide resolved
@alpe alpe force-pushed the alex/group_start branch 2 times, most recently from 2be53be to e617156 Compare February 18, 2020 14:27
@alpe alpe changed the title Alex/group start Create Group implementation Feb 19, 2020
@alpe alpe mentioned this pull request Feb 20, 2020
Closed
@alpe
Copy link
Collaborator Author

alpe commented Feb 21, 2020

🚨 This PR blocks the other group PRs.

@alpe alpe changed the base branch from module/group to orm_develop February 26, 2020 16:24
@alpe alpe mentioned this pull request Mar 3, 2020
Open
@alpe alpe force-pushed the alex/group_start branch from e617156 to 5108be2 Compare March 3, 2020 09:57
@alpe alpe changed the base branch from orm_develop to new_master March 3, 2020 09:57
@alpe
Copy link
Collaborator Author

alpe commented Mar 3, 2020

This has been pending for a while already while I was working mostly on #24 .
I have created a couple of smaller task and todos that will eventually go into github issues to reduce WIP. Addressing Amino would be done in a new PR.

@alpe alpe marked this pull request as ready for review March 3, 2020 16:46
@alpe alpe requested a review from aaronc March 3, 2020 16:46
@alpe alpe mentioned this pull request Mar 3, 2020
Merged
aaronc
aaronc approved these changes Mar 3, 2020
View reviewed changes
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

This looks fine. Left a few comments, nothing blocking and want to keep this moving forward, so merging.

incubator/group/handler.go
sdk.EventTypeMessage,
sdk.NewAttribute(sdk.AttributeKeyModule, ModuleName),
sdk.NewAttribute(sdk.AttributeKeySender, msg.Admin.String()),
),
Copy link
Member

Choose a reason for hiding this comment

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

Should we include the new group ID as an event? Not blocking, just wondering

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think now adding events can be a task of it's own.

incubator/group/types.go

type GroupID uint64

func (g GroupID) Byte() []byte {
Copy link
Member

Choose a reason for hiding this comment

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

This should be plural Bytes, but I believe it's an ORM thing and we can address in a separate PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ja, plural. will add to #24

@aaronc aaronc merged commit 9c900c4 into regen-network:new_master Mar 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@aaronc aaronc aaronc approved these changes

Assignees
No one assigned
Labels
Group module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alpe @aaronc