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 account related txs and events #277

Merged
merged 1 commit into from
Jan 17, 2023
Merged

Conversation

Andrew7234
Copy link
Collaborator

@Andrew7234 Andrew7234 commented Jan 10, 2023

I compiled+tested this prior to rebasing onto the openapi generated type changes. It will need to be rebased again once the codegen follow-up changes are merged.

It might be useful to go commit by commit, but only for the first 4 commits.

Open questions:

  • The openapi spec calls consensus events ConsensusEvents but consensus transactions Transaction, which seems inconsistent. I'm in favor of renaming to Events, but we could also rename Transaction->ConsensusTransaction
  • Since we support filtering events by type, we need to have some mapping of all the valid event types. This used to be adjacent to the Event type the api types, but now that event types are autogenerated the mapping is separate from the ground truth. This could(?) drift, but it seems like this would be a rare occurence.
  • I'd like to go ahead and rename all block fields -> height. We discussed it at the last product design meeting of December and the follow up was to do some user research, but the general sentiment seemed to be that height >> block. I'll do this in a separate PR, but for now the new type introduced uses Event.Height.

@Andrew7234 Andrew7234 force-pushed the andrew7234/account-activity branch 2 times, most recently from c74b201 to d387207 Compare January 12, 2023 01:41
@Andrew7234 Andrew7234 marked this pull request as ready for review January 12, 2023 01:50
Copy link
Contributor

@mitjat mitjat left a comment

Choose a reason for hiding this comment

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

Thanks Andy! I mostly only have stylistic suggestions, which should be easy to address. The only exception is the very last comment, on the structure of the accounts_related_events table.

analyzer/api.go Outdated Show resolved Hide resolved
analyzer/consensus/consensus.go Outdated Show resolved Hide resolved
analyzer/consensus/consensus.go Outdated Show resolved Hide resolved
analyzer/consensus/consensus.go Outdated Show resolved Hide resolved
api/spec/v1.yaml Show resolved Hide resolved
api/v1/handlers.go Outdated Show resolved Hide resolved
storage/migrations/01_oasis_3_consensus.up.sql Outdated Show resolved Hide resolved
storage/migrations/01_oasis_3_consensus.up.sql Outdated Show resolved Hide resolved
storage/migrations/01_oasis_3_consensus.up.sql Outdated Show resolved Hide resolved
storage/client/queries.go Outdated Show resolved Hide resolved
@mitjat
Copy link
Contributor

mitjat commented Jan 12, 2023

Regarding the open questions from the PR description:

  • The openapi spec calls consensus events ConsensusEvents but consensus transactions Transaction, which seems inconsistent. I'm in favor of renaming to Events, but we could also rename Transaction->ConsensusTransaction

I've been wanting to do the Transaction->ConsensusTransaction rename myself already :). I vote for that one, since in some senses, consensus is analogous to yet another (special) runtime. In general, I prefer saying ConsensusFoo over Foo, because I don't think it's obvious that Foo relates to consensus. This applies less to consensus specifics like governance voting, but definitely applies to generic things like events.

  • Since we support filtering events by type, we need to have some mapping of all the valid event types. This used to be adjacent to the Event type the api types, but now that event types are autogenerated the mapping is separate from the ground truth. This could(?) drift, but it seems like this would be a rare occurence.

Yeah I think we're OK. We could add a comment? In #281, I argue we don't even need the full list, because checking for a valid enum input is maybe more work than it's worth. My opinion would be different if Go had better supprot for enums, or of oapi-codegen supproted them better. But really I don't care very much either way; I'm fine with a different solution to the one I'm proposing.

  • I'd like to go ahead and rename all block fields -> height. We discussed it at the last product design meeting of December and the follow up was to do some user research, but the general sentiment seemed to be that height >> block. I'll do this in a separate PR, but for now the new type introduced uses Event.Height.

My intuition is to go the other way; in fact, I think I recently renamed height->block somehere, maybe in txs :). A block has a height, but a tx appears in a block (to which we happen to refer with a height). It's like how an event refers to a tx (well kinda ... tx_index really, because FKs are complicated there), not just to a hash or index., which are properties of the tx. We could rename to block_height? It's a little verbose. Anyway, let's talk during the next group meeting.

@aefhm
Copy link
Contributor

aefhm commented Jan 13, 2023

Open questions:

  • The openapi spec calls consensus events ConsensusEvents but consensus transactions Transaction, which seems inconsistent. I'm in favor of renaming to Events, but we could also rename Transaction->ConsensusTransaction

Agree with @mitjat on Transaction->ConsensusTransaction

  • Since we support filtering events by type, we need to have some mapping of all the valid event types. This used to be adjacent to the Event type the api types, but now that event types are autogenerated the mapping is separate from the ground truth. This could(?) drift, but it seems like this would be a rare occurence.

I'm also okay with a comment.

  • I'd like to go ahead and rename all block fields -> height. We discussed it at the last product design meeting of December and the follow up was to do some user research, but the general sentiment seemed to be that height >> block. I'll do this in a separate PR, but for now the new type introduced uses Event.Height.

I can go either way. Probably also defer to block.

@Andrew7234 Andrew7234 force-pushed the andrew7234/account-activity branch from 678bed3 to 02cbf69 Compare January 14, 2023 02:25
Copy link
Contributor

@mitjat mitjat left a comment

Choose a reason for hiding this comment

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

Looking good! Thank you!

storage/client/client.go Show resolved Hide resolved
analyzer/api.go Outdated Show resolved Hide resolved
analyzer/consensus/consensus.go Outdated Show resolved Hide resolved
analyzer/consensus/consensus.go Outdated Show resolved Hide resolved
analyzer/consensus/consensus.go Show resolved Hide resolved
storage/client/queries.go Outdated Show resolved Hide resolved
storage/migrations/01_oasis_3_consensus.up.sql Outdated Show resolved Hide resolved
storage/migrations/01_oasis_3_consensus.up.sql Outdated Show resolved Hide resolved
storage/migrations/01_oasis_3_consensus.up.sql Outdated Show resolved Hide resolved
…/transactions

[db] add rel events/transactions for consensus

[analyzer] index account-related events/txs

[api] impl /consensus/events endpoint; rel param for /consensus/transactions

[openapi] adjust event type

nits

remove obsolete Event enum in favor of autogenerated one

[analyzer] store rel addresses in text[] col

[api] update rel events/txs

address comments

oapi linter

address comments
@Andrew7234 Andrew7234 force-pushed the andrew7234/account-activity branch from 863090f to 7e2af40 Compare January 17, 2023 03:32
@Andrew7234 Andrew7234 merged commit cca9868 into main Jan 17, 2023
@Andrew7234 Andrew7234 deleted the andrew7234/account-activity branch January 17, 2023 03:42
@Andrew7234 Andrew7234 mentioned this pull request Jan 17, 2023
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.

3 participants