-
Notifications
You must be signed in to change notification settings - Fork 164
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
fix: use entry uuid uniformly in return responses #1012
Conversation
pkg/api/entries.go
Outdated
@@ -96,6 +96,13 @@ func logEntryFromLeaf(ctx context.Context, signer signature.Signer, tc TrillianC | |||
} | |||
|
|||
uuid := hex.EncodeToString(leaf.MerkleLeafHash) | |||
activeTree := fmt.Sprintf("%x", tc.logID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we guaranteed to only be pulling from the activeTree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I assumed the passed tc
was the one who's log ID would be matching, but I also see a tid
parameter in this function.
Regardless, if the comment was just on naming, yes, i don't think this is always the active tree. will update
65126ad
to
6f5383a
Compare
Trying to figure out the json end of line issue with attestations... |
2a01a65
to
bb0ed29
Compare
Fixed, it was an error handling mistake! I thought |
@lkatalin could i get your LGTM on this? I'm not sure if I'm breaking this comment. I'm allowing long EntryUUIDs to be passed in
|
I think that comment addresses an earlier change in which EntryIDs would be shortened to UUIDs since they were unsupported. That should have been fixed in #671, so I think this change is safe. This LGTM, think it just needs a rebase. |
bb0ed29
to
a5b6b6e
Compare
all set! (or I can ninja remove that comment since it seems to no longer apply?) |
up to you! i'll lgtm for now & i can lgtm again if you want to remove it |
Signed-off-by: Asra Ali <asraa@google.com> update Signed-off-by: Asra Ali <asraa@google.com> update tree id Signed-off-by: Asra Ali <asraa@google.com> update Signed-off-by: Asra Ali <asraa@google.com> fix errors Signed-off-by: Asra Ali <asraa@google.com> update Signed-off-by: Asra Ali <asraa@google.com>
a5b6b6e
to
0633cbb
Compare
this change lgtm, I wonder when/if it would be good to update openapi.yaml to only accept the full entryID (80 characters) instead of the 64 character leaf hash we started with. |
I'll file a tracking issue for this for some discussion. i THINK people will either have the full entry UUID, OR they will be querying by proposed entry or log index, so i think it will be safe to start restricting but not sure. |
Signed-off-by: Asra Ali asraa@google.com
Summary
Fixes #980
Release Note
Documentation
@priyawadhwa could you please take a look at sharding tests to make sure i didn't omit something purposely tested?