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

Adding SkipchainDB.GetProofFromIndex #2423

Merged
merged 2 commits into from Dec 10, 2020
Merged

Adding SkipchainDB.GetProofFromIndex #2423

merged 2 commits into from Dec 10, 2020

Conversation

ineiti
Copy link
Member

@ineiti ineiti commented Dec 8, 2020

Previously the GetByID was only available as a service-endpoint, but as it is
a handy method in general, this PR moves it to the SkipchainDB as GetFullProof,
which can do much more.

Also contains a fix to SkipBlock.pathForIndex

@ineiti ineiti self-assigned this Dec 8, 2020
@ineiti ineiti added this to WIP in Cothority via automation Dec 8, 2020
@ineiti ineiti mentioned this pull request Dec 8, 2020
@ineiti ineiti moved this from WIP to Ready4Merge in Cothority Dec 9, 2020
@ineiti ineiti requested a review from tharvik December 9, 2020 09:43
})
// GetProof returns the shortest chain from the genesis to the latest block
// using the highest forward-links available in the local db
func (db *SkipBlockDB) GetProof(sid SkipBlockID) (sbs []*SkipBlock,
Copy link
Contributor

@tharvik tharvik Dec 9, 2020

Choose a reason for hiding this comment

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

As it is pretty much the same as GetFullProof, maybe it can be removed?

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'm hesitating - it is public, so theoretically it could be used elsewhere, and I didn't want to break it. But looking at the usage, it's only in the skipchain package, so it should be fine. I can mark it Deprecated (even though we never really removed anything, did we?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm hesitating - it is public, so theoretically it could be used elsewhere, and I didn't want to break it. But looking at the usage, it's only in the skipchain package, so it should be fine.

As you wish, I'm always in favor of reducing the number of exposed function, especially if it serves the same purpose.

I can mark it Deprecated (even though we never really removed anything, did we?)

Yeah, to be nice with semver, we should do a new major release where we remove the deprecated functions, but that might never happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed the methods to make more sense:

  • GetProofFromIndex - takes a skipchain-ID and an index
  • GetProof - for compatibility, deprecated, not used anymore in the code
  • GetProofForLatest - calls GetProofFromIndex with magical value -1 - replaces previous GetProof

And the return values of all these methods is now the same. The Proof structure got a GetForwardLinks method.

@tharvik
Copy link
Contributor

tharvik commented Dec 9, 2020

And there is a Code Smell for the naming convention of test case. btw, I finally found my "golang standard" argument, in effective go.

@ineiti
Copy link
Member Author

ineiti commented Dec 9, 2020

OK, bikeshedding for TestSkipBlockDB_GetFullProof:

So my take here is: follow the sheeps. dedis/cothority has TestStruct_Method in many places already, so just accept it.

@tharvik
Copy link
Contributor

tharvik commented Dec 9, 2020

So my take here is: follow the sheeps. dedis/cothority has TestStruct_Method in many places already, so just accept it.

Okay, then let's keep it as is, but you'll need to fix SonarCloud to avoid the next code smells :P

@cgrigis
Copy link
Collaborator

cgrigis commented Dec 9, 2020

Agreed, whatever the accepted rules are, the tools should be adjusted accordingly to remove false positives. :)

@ineiti
Copy link
Member Author

ineiti commented Dec 9, 2020

Opened #2425 so we can go forward with this.

Previously the GetByID was only available as a service-endpoint, but as it is
a handy method in general, this PR moves it to the SkipchainDB as `GetFullProof`,
which can do much more.

Also contains a fix to SkipBlock.pathForIndex
- GetFullProof -> GetProofFromIndex
- GetProof -> GetProofFromLatest
- GetProof: Deprecated

Also changed the return-values from the GetProofs to always return a Proof.
Added GetForwardLinks to the Proof structure.
Added tests
@ineiti
Copy link
Member Author

ineiti commented Dec 10, 2020

OK - changed the names and signatures of the GetProof* methods and added some tests.

@sonarcloud
Copy link

sonarcloud bot commented Dec 10, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ineiti ineiti changed the title Adding SkipchainDB.GetFullProof Adding SkipchainDB.GetProofFromIndex Dec 10, 2020
@ineiti ineiti requested a review from tharvik December 10, 2020 07:01
@tharvik tharvik merged commit 1887e39 into master Dec 10, 2020
Cothority automation moved this from Ready4Merge to Closed Dec 10, 2020
@tharvik tharvik deleted the get_full_proof branch December 10, 2020 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Cothority
  
Closed
Development

Successfully merging this pull request may close these issues.

None yet

3 participants