cerc-io/laconicd-deprecated
13
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: :main
Conversation 28 Commits 35 Files Changed 20
+4449 -260
0xmuralik commented 2023-01-11 11:00:51 +00:00 (Migrated from github.com)
Copy Link
Copy Source

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
i-norden left a comment
Member
Copy Link
Copy Source

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
Copy Source

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
Copy Source

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
Copy Source

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
Copy Source

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
Copy Source

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
Copy Source

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
Copy Source

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
Copy Source

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
Copy Source

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
Copy Source

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
Copy Source

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
Copy Source

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
Copy Source

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
Copy Source

@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
Copy Source

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
Copy Source

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
Copy Source

@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
Copy Source

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

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

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

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

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
Copy Source

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.
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
Copy Source

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
Copy Source

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
Copy Source

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
Copy Source

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
Copy Source

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
Copy Source

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
Copy Source

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 left a comment
Member
Copy Link
Copy Source

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
Copy Source

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 left a comment
Member
Copy Link
Copy Source

@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
Copy Source

@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
Copy Source

@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
Copy Source

@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

@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
Copy Source

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
Copy Source

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
Copy Source

@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
Copy Source

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
Copy Source

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
ABastionOfSanity
aleem1314
0xmuralik
github-code-scanning[bot]
Labels
Clear labels
C:CLI
C:Crypto
C:Encoding
C:Proto
C:Types
Status: Stale
Type: ADR
Type: Build
Type: CI
Type: Docs
Type: Tests
bug
Something isn't working
 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
 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 labels C:CLI C:Proto
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
AFDudley (A. F. Dudley) Alex ashwin cerccicd ericlewis i-norden (Ian Norden) iskay prathamesh (Prathamesh Musale) rachmaninovquar rjk-laconic roysc (Roy Crihfield) srw (srw) zramsay
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: cerc-io/laconicd-deprecated#79