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

Retrieve Attestations from Operation Service #1606

Merged
merged 26 commits into from
Feb 15, 2019
Merged

Conversation

terencechain
Copy link
Member

@terencechain terencechain commented Feb 15, 2019

This is part of #1565

Key changes:

  • Implemented functions to retrieve all the attestations and delete attestation in package db
  • Implemented function to retrieve attestation up to max attestation amount and delete retrieved attestations in package operation as part of operation service receiver
  • Implemented tests for the above

What's left:

  • RPC server needs to interface with operation service to retrieve these attestations. func (s *Service) Attestations() ([]*pb.Attestation, error)

if err != nil {
return err
}
attestations = append(attestations, attestation)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we cap it off at max attestations here? What if we have a million records in the db, this method will fetch them all

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather keep this function as generic as possible so anyone can use it w/o having to worry it's capped or not. I think the clear separation of responsibility is nice (DB retrieves it, and Operation service caps it).
In terms of millions records in the DB, we have clean up routines that deletes attestation after it's seen from the block, and we only keep attestations up to last finalized block.

Copy link
Member Author

@terencechain terencechain Feb 15, 2019

Choose a reason for hiding this comment

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

Will update comment to be more explicit, these are the attestations that have not been included on chain

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, I see the use case where we might want to fetch all of them perhaps for visualizations or metrics gathering

Copy link
Contributor

@rauljordan rauljordan 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, just a comment

@codecov
Copy link

codecov bot commented Feb 15, 2019

Codecov Report

Merging #1606 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1606   +/-   ##
=======================================
  Coverage   71.61%   71.61%           
=======================================
  Files          94       94           
  Lines        6568     6568           
=======================================
  Hits         4704     4704           
  Misses       1463     1463           
  Partials      401      401

@rauljordan rauljordan merged commit c8a170d into master Feb 15, 2019
@rauljordan rauljordan deleted the retrieve-attestations branch February 15, 2019 19:27
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.

2 participants