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

feat(ipns): helper ValidateWithPeerID and UnmarshalIpnsEntry #294

Merged
merged 1 commit into from
May 10, 2023

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented May 10, 2023

After my overly excited merge and revert of #292, I looked at the code and saw we were already duplicating existing code. E.g. the key extraction logic already existed and was being duplicated. This PR adds two helper functions ValidateWithPeerID and UnmarshalIpnsEntry. They already use existing functionality. I also added tests.

I did not include IpnsInspectEntry for the reasons here: #292 (comment) - it doesn't make sense for us to be adding it here since it is tightly related to the type we use in ipfs name inspect. We only created it in first place to simplify the type used by the RPC client in Kubo.

I also enabled a test that has been disabled for 5 years in 0320605. It seems to pass now, and reading the commit description, I don't think it is a problem due to the way Peer IDs are encoded nowadays.

Tested in Kubo here: ipfs/kubo#9867

@hacdias hacdias requested review from lidel and a team as code owners May 10, 2023 09:29
@hacdias hacdias changed the title feat: reusable ipns verify (#292) feat(ipns): helper ValidateWithPeerID and UnmarshalIpnsEntry May 10, 2023
@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Merging #294 (f9b6003) into main (dc731ca) will decrease coverage by 0.02%.
The diff coverage is 21.42%.

❗ Current head f9b6003 differs from pull request most recent head 50d2bc2. Consider uploading reports for the commit 50d2bc2 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #294      +/-   ##
==========================================
- Coverage   48.02%   48.00%   -0.02%     
==========================================
  Files         279      279              
  Lines       33439    33451      +12     
==========================================
  Hits        16059    16059              
- Misses      15694    15703       +9     
- Partials     1686     1689       +3     
Impacted Files Coverage Δ
gateway/handler_ipns_record.go 0.00% <0.00%> (ø)
ipns/ipns.go 55.83% <23.07%> (-1.58%) ⬇️

... and 8 files with indirect coverage changes

@hacdias hacdias force-pushed the feat/reusable-ipns-validation branch 3 times, most recently from 2e10687 to 88ba464 Compare May 10, 2023 10:39
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

LGTM once thoses minors things are fixed.

ipns/validate_test.go Outdated Show resolved Hide resolved
ipns/validate_test.go Outdated Show resolved Hide resolved
ipns/validate_test.go Outdated Show resolved Hide resolved
ipns/validate_test.go Outdated Show resolved Hide resolved
ipns/validate_test.go Outdated Show resolved Hide resolved
ipns/validate_test.go Outdated Show resolved Hide resolved
ipns/validate_test.go Outdated Show resolved Hide resolved
@hacdias hacdias force-pushed the feat/reusable-ipns-validation branch from 0b41ecc to 50d2bc2 Compare May 10, 2023 11:17
@hacdias hacdias requested a review from Jorropo May 10, 2023 11:17
@hacdias hacdias enabled auto-merge (squash) May 10, 2023 11:21
@hacdias hacdias disabled auto-merge May 10, 2023 11:40
@hacdias hacdias merged commit 33e3f0c into main May 10, 2023
@hacdias hacdias deleted the feat/reusable-ipns-validation branch May 10, 2023 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants