feat(x/data)!: support public resolvers for IPFS support #2098

aaronc · 2023-11-29T18:27:42Z

Description

This PR:

adds the bool public field to MsgDefineResolver
renames MsgDefineResolver manager to definer
renames MsgRegisterResolver manager to signer

These last two proto breaking changes are okay because we are currently working with the as of yet unreleased regen.data.v2 API.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

clevinson

@aaronc So I prematurely approved, and then after closer inspection actually have a few questions about how we expect this be used by users.

clevinson · 2024-03-06T00:54:22Z

proto/regen/data/v1/tx.proto

@@ -112,11 +112,23 @@ message MsgDefineResolver {
  // the manager should make use of cosmos.authz.
  string manager = 1;


Who would be the manager for the ipfs: resolver?

Can you elaborate on how you would expect this to work? In reading the documentation for the manager field, it seems like only the manager of a resolver can register data to a given resolver.

So as I understand it, this PR implies that each user wanting to host data on IPFS would define their own resolver named ipfs: ?

I wonder if it would make more sense to instead extend the MsgRegisterResolver() message to have an enum field called resolver_type. we could have the default / 0 value of that enum be HTTP_RESOLVER, and 1 = IPFS_RESOLVER. This way, if we want to support other decentralized resolvers that are similar to IPFS in the future, we could just extend this enum?

I think you're right. So, there's another step of work to do here. I don't think it will be too hard. I will close this, work on that and ping you when it's R4R again.

@clevinson I've updated this PR to support marking a resolver as public to support IPFS. Please take a look again when you get a chance.

codecov · 2024-03-06T20:34:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.91%. Comparing base (3b21ecd) to head (be0f559).
Report is 1 commits behind head on main.

❗ Current head be0f559 differs from pull request most recent head 1d53b64. Consider uploading reports for the commit 1d53b64 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2098   +/-   ##
=======================================
  Coverage   72.90%   72.91%           
=======================================
  Files         240      240           
  Lines       13860    13865    +5     
=======================================
+ Hits        10105    10110    +5     
  Misses       3016     3016           
  Partials      739      739

Files	Coverage Δ
x/data/server/msg_define_resolver.go	`72.00% <100.00%> (+5.33%)`	⬆️
x/data/server/msg_register_resolver.go	`69.23% <100.00%> (+0.80%)`	⬆️

clevinson · 2024-03-07T18:00:06Z

Reading through the updates, it looks like the approach you're intending for here is still that the defining of an IPFS resolver is something that is done in state (via someone defining an IPFS resolver via a MsgDefineResolver() tx).

So is your idea that someone from the RND team would submit a tx define a resolver for IPFS shortly after this software upgrade happens? Or perhaps we would just bake that into a state migration and ship it along w/ the software upgrade?

In either case though, I don't think we have a uniqueness constraint on resolver_url, so there's nothing preventing multiple users from creating new "IPFS" resolvers with the same resolver_url and public = true. If and when this happens, for a user wanting to register their data to IPFS, they now would be presented with several different options of which resolvers to register with, all seemingly the same.

I'm just trying to unpack this here and make sure you're tracking this edge case and that you think this is OK @aaronc. Personally it feels a bit clunky and I'd prefer an approach where as a user wanting to register data to IPFS, there is one clear way to do that, and there's no chance of several different "resolvers" in state that all in practice mean identical things. But if there are reasons why you thinks approach is better than what I wrote in my comment (#2098 (comment)), which I think addresses the issue of end-users only having at most 1 way to register their data with IPFS, let me know.

aaronc · 2024-03-07T18:49:07Z

There is a uniqueness check on the resolver,manager pair even when manager is nil in the case of being public. So I don't think double definitions are possible. I can add a test case to check

aaronc · 2024-03-07T19:45:51Z

x/data/server/features/msg_define_resolver.feature

+    Scenario: public resolvers can only be defined once per URL
+      Given a public resolver is defined for the url "ipfs:"
+      When alice attempts to define a public resolver with url "ipfs:"
+      Then expect the error "a resolver with the same URL and manager already exists: unique key violation"


@clevinson I added a test here which demonstrates that a public resolver URL can only be defined once.

paul121

This LGTM, just one question to clarify how an IPFS CID would be used.

paul121 · 2024-03-12T15:09:45Z

proto/regen/data/v2/tx.proto

+  // To use IPFS, the resolver_url ipfs: should be defined with public set to true
+  // and used as the resolver for any data hosted on IPFS. Content hashes must be
+  // adapted to IPFS's CID format. The multicodec raw (0x55) should


Content hashes must be adapted to IPFS's CID format.

Just want to clarify how this works.. For both raw and graph content hashes, the hash will be set to the full cid, where the cid was generated using either the multicodec raw (0x55) or multicodec rdfc-1 (0xb403)?

The content hash on Regen would still be the same as if it were registered w/ an HTTP resolver. I think this comment just communicates that users must take the resulting Regen IRI, and convert it to a specific IPFS CID in order to fetch the data from the IPFS network.

Ah thanks, this makes more sense. With an example I think this would be very clear. Hopefully soon...

aaronc added 4 commits November 29, 2023 13:27

docs(x/data): document IPFS resolver support

78ff598

update IPFS docs

db0a7f3

Merge branch 'main' into aaronc/ipfs-resolver

b699d23

list caveats

f35e6f6

aaronc marked this pull request as ready for review March 5, 2024 22:28

clevinson approved these changes Mar 6, 2024

View reviewed changes

clevinson self-requested a review March 6, 2024 00:41

clevinson reviewed Mar 6, 2024

View reviewed changes

aaronc closed this Mar 6, 2024

aaronc added 2 commits March 6, 2024 15:21

add public field

bf550a3

test case

c67eb10

aaronc changed the title ~~docs(x/data): document IPFS resolver support~~ feat(x/data)!: support public resolvers for IPFS support Mar 6, 2024

aaronc reopened this Mar 6, 2024

aaronc changed the title ~~feat(x/data)!: support public resolvers for IPFS support~~ feat(x/data): support public resolvers for IPFS support Mar 6, 2024

aaronc changed the title ~~feat(x/data): support public resolvers for IPFS support~~ feat(x/data)!: support public resolvers for IPFS support Mar 6, 2024

add test to ensure public resolvers can only be defined once per URL

be0f559

aaronc commented Mar 7, 2024

View reviewed changes

clevinson approved these changes Mar 7, 2024

View reviewed changes

paul121 reviewed Mar 12, 2024

View reviewed changes

paul121 approved these changes Mar 13, 2024

View reviewed changes

Merge branch 'main' into aaronc/ipfs-resolver

1d53b64

aaronc enabled auto-merge (squash) March 14, 2024 18:43

aaronc merged commit 0cda6aa into main Mar 14, 2024
26 of 27 checks passed

aaronc deleted the aaronc/ipfs-resolver branch March 14, 2024 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(x/data)!: support public resolvers for IPFS support #2098

feat(x/data)!: support public resolvers for IPFS support #2098

aaronc commented Nov 29, 2023 •

edited

Loading

clevinson left a comment

clevinson Mar 6, 2024

aaronc Mar 6, 2024

aaronc Mar 6, 2024

codecov bot commented Mar 6, 2024 •

edited

Loading

clevinson commented Mar 7, 2024

aaronc commented Mar 7, 2024

aaronc Mar 7, 2024

paul121 left a comment

paul121 Mar 12, 2024

clevinson Mar 12, 2024

aaronc Mar 12, 2024

paul121 Mar 12, 2024

		@@ -112,11 +112,23 @@ message MsgDefineResolver {
		// the manager should make use of cosmos.authz.
		string manager = 1;

feat(x/data)!: support public resolvers for IPFS support #2098

feat(x/data)!: support public resolvers for IPFS support #2098

Conversation

aaronc commented Nov 29, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

clevinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 6, 2024 • edited Loading

Codecov Report

clevinson commented Mar 7, 2024

aaronc commented Mar 7, 2024

Choose a reason for hiding this comment

paul121 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc commented Nov 29, 2023 •

edited

Loading

codecov bot commented Mar 6, 2024 •

edited

Loading