Skip to content
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

Add Events for personal feed #379

Merged
merged 13 commits into from
Nov 5, 2019
Merged

Add Events for personal feed #379

merged 13 commits into from
Nov 5, 2019

Conversation

ben-kaufman
Copy link
Contributor

Fix #377.

src/domain/index.ts Outdated Show resolved Hide resolved
@ben-kaufman ben-kaufman marked this pull request as ready for review October 31, 2019 10:30
type Event @entity {
id: ID!
type: EventType!
data: String!
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this data field ? why it is needed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That include some JSON data Alchemy might need. @jellegerbrandy

Copy link
Contributor

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

you can see what goes into the data field in the various addXXXEvent functions above. we need to tell the users what happened for each event, like the amount staked.

Copy link
Contributor

@orenyodfat orenyodfat Oct 31, 2019

Choose a reason for hiding this comment

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

is that cannot be analyzed from the event it self ? in the client side?

Copy link
Contributor

Choose a reason for hiding this comment

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

addEvent(
'NewDAO',
avatar.toHex(),
'{ "address": "' + avatar.toHex() + '", "name": "' + daoName + '" }',
Copy link
Contributor

Choose a reason for hiding this comment

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

what is that ?
why it is hard coded here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That’s just the data object Alchemy needs for this event type...

Copy link
Contributor

Choose a reason for hiding this comment

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

on first glance it looks not so elegant solution. is that can be avoided ? @jellegerbrandy

Choose a reason for hiding this comment

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

I think we don't need the Dao avatar address here since we already have that in its own field.

Copy link
Contributor

Choose a reason for hiding this comment

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

tibet is right: the avatar is not strictly needed (although neither does it hurt to have it :-))

I do not really understand @orenyodfat's objection.
Events have a data attribute that is a string. This string happends to be a JSON structure, which is useful when we want to reprseent the event. This does not seem particularly unelegant to me as a pattern

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this data need to be searchable and indexed?
It looks too specific and duplicate the field we have on the entity it self .

Copy link
Contributor

Choose a reason for hiding this comment

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

no it does not, that is why it is in a text field and not in its own separate field.

@ben-kaufman ben-kaufman changed the title Add NewProposal event. Add Events for personal feed Oct 31, 2019
Copy link
Contributor

@jellegerbrandy jellegerbrandy left a comment

Choose a reason for hiding this comment

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

looks good to me!

src/domain/schema.graphql Show resolved Hide resolved
Copy link
Contributor

@orenyodfat orenyodfat left a comment

Choose a reason for hiding this comment

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

@orenyodfat
Copy link
Contributor

orenyodfat commented Oct 31, 2019 via email

@orenyodfat
Copy link
Contributor

orenyodfat commented Nov 4, 2019 via email

@ben-kaufman ben-kaufman merged commit b1a5958 into master Nov 5, 2019
@ben-kaufman ben-kaufman deleted the events-feed branch November 5, 2019 13:10
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.

Personal feed requirements
4 participants