cerc-io/laconicd-deprecated
19
0
Fork 1
Code Issues 21 Pull Requests Actions Packages Projects Releases 3 Wiki Activity

feat: remaining record content types to support #79

Closed
0xmuralik wants to merge 35 commits from murali/record-attributes into main
pull from: murali/record-attributes
merge into: cerc-io:main
Conversation 10 Commits 35 Files Changed 20 +4449 -260
0xmuralik commented 2023-01-11 11:00:51 +00:00 (Migrated from github.com)
Copy Link

Closes: #51

Description

Record attributes indexing for remaining defined type of records.

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)
<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺ v ✰ Thanks for creating a PR! ✰ v Before smashing the submit button please review the checkboxes. v If a checkbox is n/a - please still include it but + a little note why ☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --> Closes: #51 ## Description Record attributes indexing for remaining defined type of records. ______ For contributor use: - [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`) - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md` - [ ] Re-reviewed `Files changed` in the Github PR explorer ______ For admin use: - [ ] Added appropriate labels to PR (ex. `WIP`, `R4R`, `docs`, etc) - [ ] Reviewers assigned - [ ] Squashed all commits, uses message "Merge pull request #XYZ: [title]" ([coding standards](https://github.com/tendermint/coding/blob/master/README.md#merging-a-pr))
ABastionOfSanity (Migrated from github.com) reviewed 2023-01-11 11:00:51 +00:00
aleem1314 (Migrated from github.com) reviewed 2023-01-11 11:00:51 +00:00
zramsay reviewed 2023-01-11 11:00:51 +00:00
i-norden reviewed 2023-01-11 11:00:51 +00:00
i-norden requested changes 2023-01-12 22:52:58 +00:00
i-norden left a comment
Member
Copy Link

This all looks great! I made some comments to fill in a few fields that were missing in the specification or under/mis-specified, also suggest clarifying which fields are content hash references (renaming to e.g. hash_reference) so went through and noted all of the linkages.

This all looks great! I made some comments to fill in a few fields that were missing in the specification or under/mis-specified, also suggest clarifying which fields are content hash references (renaming to e.g. `hash_reference`) so went through and noted all of the linkages.
proto/vulcanize/registry/v1beta1/attributes.proto
@ -23,3 +29,3 @@
}
message WebsiteRegistrationRecord {
i-norden commented 2023-01-12 22:15:45 +00:00
Member
Copy Link

This was underspecified on my part, instead of a raw_multicodec field and a raw_binary field, all we need is a hash_reference field. And this reference will be a CID or multihash which uses the raw mutliformat prefix and references the binary as an IPLD object.

This was underspecified on my part, instead of a `raw_multicodec` field and a `raw_binary` field, all we need is a `hash_reference` field. And this reference will be a CID or multihash which uses the [raw mutliformat prefix](https://github.com/multiformats/multicodec/blob/master/table.csv#L40) and references the binary as an IPLD object.
i-norden commented 2023-01-12 22:26:47 +00:00
Member
Copy Link

Let's rename this to JSPackage and add a string name=5; field

Let's rename this to `JSPackage` and add a `string name=5;` field
i-norden commented 2023-01-12 22:34:49 +00:00
Member
Copy Link

This will reference a registered GitRepository record by CID/multihash

GitRepository also has a repo_reference field which will contain a qualitative/non-content-hash-reference e.g. a github or gitea web url.

It might be helpful to signal in the naming of the various reference fields which references are content-hash => content references vs one's that are not. E.g. hash_reference vs reference.

This will reference a registered `GitRepository` record by CID/multihash `GitRepository` also has a `repo_reference` field which will contain a qualitative/non-content-hash-reference e.g. a github or gitea web url. It might be helpful to signal in the naming of the various `reference` fields which references are content-hash => content references vs one's that are not. E.g. `hash_reference` vs `reference`.
i-norden commented 2023-01-12 22:42:26 +00:00
Member
Copy Link

Note: hash_reference to a GitRepository IPLD object

Note: `hash_reference` to a `GitRepository` IPLD object
i-norden commented 2023-01-12 22:43:21 +00:00
Member
Copy Link

Note: hash_reference to binary IPLD object

Note: `hash_reference` to binary IPLD object
i-norden commented 2023-01-12 22:44:01 +00:00
Member
Copy Link

Note: hash_reference to a GitRepository IPLD object

Note: `hash_reference` to a `GitRepository` IPLD object
i-norden commented 2023-01-12 22:44:42 +00:00
Member
Copy Link

Note: hash_reference to a ChainRegistrationRecord IPLD object

Note: `hash_reference` to a `ChainRegistrationRecord` IPLD object
i-norden commented 2023-01-12 22:45:44 +00:00
Member
Copy Link

Note: hash_reference to a WASM IPLD object

Note: `hash_reference` to a WASM IPLD object
i-norden commented 2023-01-12 22:46:25 +00:00
Member
Copy Link

Note: hash_reference to ServiceProviderRecord IPLD object

Note: `hash_reference` to `ServiceProviderRecord` IPLD object
i-norden commented 2023-01-12 22:47:03 +00:00
Member
Copy Link

Note: hash_reference to WatcherRegistrationRecord IPLD object

Note: `hash_reference` to `WatcherRegistrationRecord` IPLD object
i-norden commented 2023-01-12 22:48:19 +00:00
Member
Copy Link

Note: hash_reference to auction result IPLD object

Note: `hash_reference` to auction result IPLD object
i-norden commented 2023-01-12 22:49:16 +00:00
Member
Copy Link

Note: hash_reference to JSPackage IPLD object

Note: `hash_reference` to `JSPackage` IPLD object
proto/vulcanize/registry/v1beta1/attributes.proto
i-norden commented 2023-01-12 22:37:22 +00:00
Member
Copy Link

Let's add fields:

string chain_id
string network_id
string genesis_hash

Let's add fields: `string chain_id` `string network_id` `string genesis_hash`
i-norden commented 2023-01-13 00:58:58 +00:00
Member
Copy Link

@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.

@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.
👍 1
0xmuralik (Migrated from github.com) reviewed 2023-01-17 05:43:47 +00:00
proto/vulcanize/registry/v1beta1/attributes.proto
@ -23,3 +29,3 @@
}
message WebsiteRegistrationRecord {
0xmuralik (Migrated from github.com) commented 2023-01-17 05:43:47 +00:00
Copy Link

There is also a npm_package_reference (CID) in this message. Should we go with renaming repo_reference with repo_hash_reference and npm_package_reference with js_package_hash_ref?

There is also a npm_package_reference (CID) in this message. Should we go with renaming repo_reference with repo_hash_reference and npm_package_reference with js_package_hash_ref?
0xmuralik (Migrated from github.com) reviewed 2023-01-17 05:56:40 +00:00
proto/vulcanize/registry/v1beta1/attributes.proto
0xmuralik (Migrated from github.com) commented 2023-01-17 05:56:40 +00:00
Copy Link

is the genesis_hash a CID reference or just a hash string?

is the genesis_hash a CID reference or just a hash string?
0xmuralik (Migrated from github.com) reviewed 2023-01-17 06:42:04 +00:00
proto/vulcanize/registry/v1beta1/attributes.proto
0xmuralik (Migrated from github.com) commented 2023-01-17 06:42:04 +00:00
Copy Link

@i-norden Can you suggest a preffered method here?

@i-norden Can you suggest a preffered method here?
0xmuralik (Migrated from github.com) reviewed 2023-01-17 06:42:21 +00:00
proto/vulcanize/registry/v1beta1/attributes.proto
0xmuralik (Migrated from github.com) commented 2023-01-17 06:42:21 +00:00
Copy Link

@i-norden Can you suggest a preffered method here?

@i-norden Can you suggest a preffered method here?
i-norden reviewed 2023-01-18 15:08:12 +00:00
proto/vulcanize/registry/v1beta1/attributes.proto
i-norden commented 2023-01-18 15:08:12 +00:00
Member
Copy Link

Good question... let's go with full CID for now

Good question... let's go with full CID for now
i-norden reviewed 2023-01-18 15:20:30 +00:00
proto/vulcanize/registry/v1beta1/attributes.proto
i-norden commented 2023-01-18 15:11:09 +00:00
Member
Copy Link

Hmm I'm not sure, it is not apparent to me what the tags or addresses fields are supposed to represent in the Tag message. I see the new examples you have created, was there an old example .yml for this type?

Hmm I'm not sure, it is not apparent to me what the `tags` or `addresses` fields are supposed to represent in the `Tag` message. I see the new examples you have created, was there an old example .yml for this type?
i-norden commented 2023-01-18 15:17:42 +00:00
Member
Copy Link

Same here unfortunately, not familiar with these types and isn't obvious to me what ref and os are supposed to represent in these mappings. Going to ask Ashwin.

Same here unfortunately, not familiar with these types and isn't obvious to me what `ref` and `os` are supposed to represent in these mappings. Going to ask Ashwin.
i-norden reviewed 2023-01-18 15:22:34 +00:00
proto/vulcanize/registry/v1beta1/attributes.proto
@ -23,3 +29,3 @@
}
message WebsiteRegistrationRecord {
i-norden commented 2023-01-18 15:22:34 +00:00
Member
Copy Link

We can avoid adding hash to the field names and instead we should revert to using a / sub-mapping to denote CID/link

We can avoid adding `hash` to the field names and instead we should revert to using a `/` sub-mapping to denote CID/link
github-code-scanning[bot] (Migrated from github.com) reviewed 2023-01-20 09:40:13 +00:00
x/registry/keeper/keeper.go
github-code-scanning[bot] (Migrated from github.com) commented 2023-01-20 09:40:12 +00:00
Copy Link

Blocklisted import reflect

Blocklisted import reflect

Show more details

## Blocklisted import reflect Blocklisted import reflect [Show more details](https://github.com/cerc-io/laconicd/security/code-scanning/566)
x/registry/keeper/keeper.go
github-code-scanning[bot] (Migrated from github.com) commented 2023-01-20 09:40:12 +00:00
Copy Link

expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 2

expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 2

Show more details

## expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 2 expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 2 [Show more details](https://github.com/cerc-io/laconicd/security/code-scanning/565)
github-code-scanning[bot] (Migrated from github.com) reviewed 2023-01-20 09:58:16 +00:00
x/registry/keeper/keeper.go
github-code-scanning[bot] (Migrated from github.com) commented 2023-01-20 09:58:16 +00:00
Copy Link

Sensitive package import

Certain system packages contain functions which may be a possible source of non-determinism

Show more details

## Sensitive package import Certain system packages contain functions which may be a possible source of non-determinism [Show more details](https://github.com/cerc-io/laconicd/security/code-scanning/598)
x/registry/keeper/keeper.go
github-code-scanning[bot] (Migrated from github.com) commented 2023-01-20 09:58:16 +00:00
Copy Link

Iteration over map

Iteration over map may be a possible source of non-determinism

Show more details

## Iteration over map Iteration over map may be a possible source of non-determinism [Show more details](https://github.com/cerc-io/laconicd/security/code-scanning/567)
codecov-commenter commented 2023-01-20 11:02:49 +00:00 (Migrated from github.com)
Copy Link

Codecov Report

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

📣 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

@@            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 Coverage Δ
x/registry/keeper/keeper.go 48.20% <52.63%> (-0.72%) ⬇️
x/registry/client/testutil/grpc.go 96.21% <100.00%> (ø)
x/registry/client/testutil/query.go 89.27% <100.00%> (ø)
x/registry/client/testutil/tx.go 96.99% <100.00%> (ø)
## [Codecov](https://codecov.io/gh/cerc-io/laconicd/pull/79?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report > Merging [#79](https://codecov.io/gh/cerc-io/laconicd/pull/79?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (bbb6d2a) into [main](https://codecov.io/gh/cerc-io/laconicd/commit/763dab712f7a623aca5c47eb09b7c1770ca62bb9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (763dab7) will **decrease** coverage by `0.01%`. > The diff coverage is `65.38%`. :mega: This organization is not using Codecov’s [GitHub App Integration](https://github.com/apps/codecov). We recommend you install it so Codecov can continue to function properly for your repositories. [Learn more](https://about.codecov.io/blog/codecov-is-updating-its-github-integration/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) <details><summary>Additional details and impacted files</summary> [![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%> (ø)` | | </details>
github-code-scanning[bot] (Migrated from github.com) reviewed 2023-01-20 11:11:56 +00:00
x/registry/keeper/keeper.go
@ -4,6 +4,7 @@ import (
"bytes"
"encoding/json"
"fmt"
"reflect" // #nosec G702 This is also used in eip712 antehandler code. No new risk introduced.
github-code-scanning[bot] (Migrated from github.com) commented 2023-01-20 11:11:55 +00:00
Copy Link

Sensitive package import

Certain system packages contain functions which may be a possible source of non-determinism

Show more details

## Sensitive package import Certain system packages contain functions which may be a possible source of non-determinism [Show more details](https://github.com/cerc-io/laconicd/security/code-scanning/599)
i-norden reviewed 2023-01-20 16:23:27 +00:00
i-norden left a comment
Member
Copy Link

Thanks Murali! Before we merge this, we need to make the corresponding changes in laconic-sdk and laconic-cli-registry and run their tests to ensure these changes work with those.

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.

Thanks Murali! Before we merge this, we need to make the corresponding changes in laconic-sdk and laconic-cli-registry and run their tests to ensure these changes work with those. 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.
0xmuralik commented 2023-02-27 09:12:44 +00:00 (Migrated from github.com)
Copy Link

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

> 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
i-norden requested changes 2023-02-28 14:59:24 +00:00
i-norden left a comment
Member
Copy Link

@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: https://github.com/cerc-io/laconicd/issues/78 (and also will be useful elsewhere).

@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: https://github.com/cerc-io/laconicd/issues/78 (and also will be useful elsewhere).
0xmuralik commented 2023-03-01 06:03:07 +00:00 (Migrated from github.com)
Copy Link

@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.

> @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 2023-03-01 15:11:35 +00:00
Member
Copy Link

@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 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.
👍 1
0xmuralik commented 2023-03-02 10:41:26 +00:00 (Migrated from github.com)
Copy Link

@i-norden @aleem1314 getting error while using map[] in txs
vulcanize.registry.v1beta1.WebsiteRegistrationRecord.RepoReferenceEntry" does not implement proto.Message at
f71df80e93/x/auth/tx/decoder.go (L40)

Same error is returned when @aleem1314 tried with a unit test inculing map[] on cosmos-sdk for f71df80e93/codec/unknownproto/unknown_fields.go (L41)

@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
👀 1
0xmuralik commented 2023-03-03 07:36:57 +00:00 (Migrated from github.com)
Copy Link

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

Submitted issue to cosmos-sdk https://github.com/cosmos/cosmos-sdk/issues/15254
👀 1
i-norden commented 2023-03-06 15:05:47 +00:00
Member
Copy Link

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.
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 2023-03-09 11:50:12 +00:00 (Migrated from github.com)
Copy Link

@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.

@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.
❤️ 1
0xmuralik commented 2023-03-16 05:18:23 +00:00 (Migrated from github.com)
Copy Link

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

See https://github.com/cerc-io/laconic-sdk/pull/36.
github-code-scanning[bot] (Migrated from github.com) reviewed 2023-03-16 05:25:52 +00:00
x/registry/keeper/keeper.go
@ -4,6 +4,7 @@ import (
"bytes"
"encoding/json"
"fmt"
"reflect" // #nosec G702 This is also used in eip712 antehandler code. No new risk introduced.
github-code-scanning[bot] (Migrated from github.com) commented 2023-03-16 05:25:52 +00:00
Copy Link

Sensitive package import

Certain system packages contain functions which may be a possible source of non-determinism

Show more details

## Sensitive package import Certain system packages contain functions which may be a possible source of non-determinism [Show more details](https://github.com/cerc-io/laconicd/security/code-scanning/1208)

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
zramsay
i-norden
aleem1314
ABastionOfSanity
Labels
Clear labels   
bug

Something isn't working

  
C:CLI

   
C:Crypto

   
C:Encoding

   
C:Proto

   
C:Types

   
dependencies

Pull requests that update a dependency file

  
docker

Pull requests that update Docker code

  
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
go

Pull requests that update Go code

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
high priority

   
in progress

currently working on

  
invalid

This doesn't seem right

  
javascript

Pull requests that update Javascript code

  
low priority

   
medium priority

   
question

Further information is requested

  
Status: Stale

   
Type: ADR

   
Type: Build

   
Type: CI

   
Type: Docs

   
Type: Tests

   
urgent

Top priority issue

  
wontfix

This will not be worked on

  
Copied from Github

An issue or PR manually copied from GitHub.

  
Kind/Breaking

Breaking change that won't be backward compatible

  
Kind/Bug

Something is not working

  
Kind/Documentation

Documentation changes

  
Kind/Enhancement

Improve existing functionality

  
Kind/Feature

New functionality

  
Kind/Security

This is security issue

  
Kind/Testing

Issue or pull request related to testing

  
Priority
Critical
The priority is critical

  
Priority
High
The priority is high

  
Priority
Low
The priority is low

  
Priority
Medium
The priority is medium

  
Reviewed
Confirmed
Issue has been confirmed

  
Reviewed
Duplicate
This issue or pull request already exists

  
Reviewed
Invalid
Invalid issue

  
Reviewed
Won't Fix
This issue won't be fixed

  
Status
Abandoned
Somebody has started to work on this but abandoned work

  
Status
Blocked
Something is blocking this issue or pull request

  
Status
Need More Info
Feedback is required to reproduce issue or to continue work

No Label
bug
C:CLI
C:Crypto
C:Encoding
C:Proto
C:Types
dependencies
docker
documentation
duplicate
enhancement
go
good first issue
help wanted
high priority
in progress
invalid
javascript
low priority
medium priority
question
Status: Stale
Type: ADR
Type: Build
Type: CI
Type: Docs
Type: Tests
urgent
wontfix
Copied from Github
Kind/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cerc-io/laconicd-deprecated#79
No description provided.
Delete Branch "murali/record-attributes"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?