cerc-io / laconicd-deprecated

Read-only mirror of https://git.vdb.to/cerc-io/laconicd-deprecated
https://git.vdb.to/cerc-io/laconicd-deprecated
GNU Lesser General Public License v3.0
6 stars 8 forks source link

feat: remaining record content types to support #79

Closed 0xmuralik closed 1 year ago

0xmuralik commented 1 year ago

Closes: #51

Description

Record attributes indexing for remaining defined type of records.


For contributor use:


For admin use:

i-norden commented 1 year ago

@0xmuralik for all of the reference fields that are hash-links, we should considering using the previous convention with "/"

E.g. npm_package_ref renamed to hash_reference (or not, since now the "/" already signifies the hash-link nature of the reference) with hash_reference a map of "/" to the CID. This should prevent the need for https://github.com/cerc-io/laconicd/issues/78.

codecov-commenter commented 1 year ago

Codecov Report

Merging #79 (bbb6d2a) into main (763dab7) will decrease coverage by 0.01%. The diff coverage is 65.38%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files [![Impacted file tree graph](https://codecov.io/gh/cerc-io/laconicd/pull/79/graphs/tree.svg?width=650&height=150&src=pr&token=5tWmn7LxfO&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)](https://codecov.io/gh/cerc-io/laconicd/pull/79?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) ```diff @@ Coverage Diff @@ ## main #79 +/- ## ========================================== - Coverage 66.39% 66.38% -0.01% ========================================== Files 148 148 Lines 16051 16042 -9 ========================================== - Hits 10657 10650 -7 + Misses 4918 4916 -2 Partials 476 476 ``` | [Impacted Files](https://codecov.io/gh/cerc-io/laconicd/pull/79?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [x/registry/keeper/keeper.go](https://codecov.io/gh/cerc-io/laconicd/pull/79?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-eC9yZWdpc3RyeS9rZWVwZXIva2VlcGVyLmdv) | `48.20% <52.63%> (-0.72%)` | :arrow_down: | | [x/registry/client/testutil/grpc.go](https://codecov.io/gh/cerc-io/laconicd/pull/79?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-eC9yZWdpc3RyeS9jbGllbnQvdGVzdHV0aWwvZ3JwYy5nbw==) | `96.21% <100.00%> (ø)` | | | [x/registry/client/testutil/query.go](https://codecov.io/gh/cerc-io/laconicd/pull/79?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-eC9yZWdpc3RyeS9jbGllbnQvdGVzdHV0aWwvcXVlcnkuZ28=) | `89.27% <100.00%> (ø)` | | | [x/registry/client/testutil/tx.go](https://codecov.io/gh/cerc-io/laconicd/pull/79?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-eC9yZWdpc3RyeS9jbGllbnQvdGVzdHV0aWwvdHguZ28=) | `96.99% <100.00%> (ø)` | |
0xmuralik commented 1 year ago

Let's hold off in this for now as Michael is working on squashing some intercompatibility bugs due to not doing this in the past, and it will be cleaner to work off of his results than to begin in parallel.

@i-norden @ABastionOfSanity @aleem1314

TODO: Merge https://github.com/cerc-io/laconic-sdk/pull/27

0xmuralik commented 1 year ago

@0xmuralik I haven't dug into this again fully yet, but before doing so let's revert back to the "/" usage for link delineation in order to fix this bug: #78 (and also will be useful elsewhere).

To use "/" we'll have to use a map for hash references. As using maps in cosmos-sdk might lead to non determinism and is not recommended, I have taken this approach. Even though there will be only one item in this map of references and we don't have any risk of non-determinisim, I thought its safer to avoid maps and just change the getReference logic in GQL to support the new HashReference type.

i-norden commented 1 year ago

@0xmuralik let's revert to using "/" please, it was a mistake on my part to suggest we move away from this standard in the first place. I appreciate the concerns for non-determinism, but as you said since it only contains a single entry this is not an issue. This will fix the issue in https://github.com/cerc-io/laconicd/issues/78 and will prevent similar issues from cropping up downstream. In general, we need some way of detecting whether or not a field in these records is a link without knowing the the specifics of the field (e.g. name), this will be useful once we move to more generic type support. Using "/" to delineate a link is also the standard in the IPLD domain.

0xmuralik commented 1 year ago

@i-norden @aleem1314 getting error while using map[] in txs vulcanize.registry.v1beta1.WebsiteRegistrationRecord.RepoReferenceEntry" does not implement proto.Message at https://github.com/cosmos/cosmos-sdk/blob/f71df80e93bffbf7ce5fbd519c6154a2ee9f991b/x/auth/tx/decoder.go#L40

Same error is returned when @aleem1314 tried with a unit test inculing map[] on cosmos-sdk for https://github.com/cosmos/cosmos-sdk/blob/f71df80e93bffbf7ce5fbd519c6154a2ee9f991b/codec/unknownproto/unknown_fields.go#L41

0xmuralik commented 1 year ago

Submitted issue to cosmos-sdk https://github.com/cosmos/cosmos-sdk/issues/15254

i-norden commented 1 year ago

Thanks for opening that issue, looks like I may have sent you on a wild goose hunt. Even if the change is acceptable I don't think we can expect the ICF team to investigate and implement the change as it is not critical to the sdk. So I think we have two options:

  1. Figure out the fix and make a patch to cosmos-sdk and attempt to upstream ourselves. I think this is the only way we can reasonably expect a change like this to make it upstream.
  2. Figure out some other way of demarcating Links in these object. Need to give this some more thought.
0xmuralik commented 1 year ago

@i-norden by adding a json tag to the ref field , we can avoid using maps and still keep "/" for referencing CIDs. The proto messages will use the Hash reference message. When the message is converted to and from json it will use a "/" instead of "ref" as the key to CID reference. The store will have record objects with "Hash Reference" proto message but the attribute keys used for gql queries will use "/" for CIDs.

0xmuralik commented 1 year ago

See https://github.com/cerc-io/laconic-sdk/pull/36.