missing unmarshalling of content bytes before encoding and generation… #86

i-norden · 2023-01-19T22:10:42Z

ABastionOfSanity commented

2023-01-19 22:10:42 +00:00

(Migrated from github.com)

This is to address
https://github.com/cerc-io/laconic-sdk/issues/7
for improperly generated CID in SetRecord endpoint.

This is to address https://github.com/cerc-io/laconic-sdk/issues/7 for improperly generated CID in SetRecord endpoint.

❤️ 1

codecov-commenter commented

2023-01-19 22:23:57 +00:00

(Migrated from github.com)

Codecov Report

Merging #86 (7db19ee) into main (ab2ea51) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #86   +/-   ##
=======================================
  Coverage   66.57%   66.57%           
=======================================
  Files         145      145           
  Lines       15953    15953           
=======================================
  Hits        10620    10620           
  Misses       4859     4859           
  Partials      474      474

Impacted Files	Coverage Δ
x/registry/keeper/keeper.go	`48.92% <100.00%> (ø)`

# [Codecov](https://codecov.io/gh/cerc-io/laconicd/pull/86?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report > Merging [#86](https://codecov.io/gh/cerc-io/laconicd/pull/86?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (7db19ee) into [main](https://codecov.io/gh/cerc-io/laconicd/commit/ab2ea51aac6e3c26a41dd4028d8d5d8884e13b50?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (ab2ea51) will **not change** coverage. > The diff coverage is `100.00%`. <details><summary>Additional details and impacted files</summary> [![Impacted file tree graph](https://codecov.io/gh/cerc-io/laconicd/pull/86/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/86?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) ```diff @@ Coverage Diff @@ ## main #86 +/- ## ======================================= Coverage 66.57% 66.57% ======================================= Files 145 145 Lines 15953 15953 ======================================= Hits 10620 10620 Misses 4859 4859 Partials 474 474 ``` | [Impacted Files](https://codecov.io/gh/cerc-io/laconicd/pull/86?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/86?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-eC9yZWdpc3RyeS9rZWVwZXIva2VlcGVyLmdv) | `48.92% <100.00%> (ø)` | | </details>

0xmuralik (Migrated from github.com) approved these changes 2023-01-23 04:51:13 +00:00

0xmuralik (Migrated from github.com) left a comment

LGTM

i-norden reviewed 2023-01-25 02:14:50 +00:00

i-norden left a comment

Thanks! Please add a unit test that shows that CIDFromJSONBytesUsingIpldPrime and CIDFromJSONBytes both match expected output(s)

Thanks! Please add a unit test that shows that `CIDFromJSONBytesUsingIpldPrime` and `CIDFromJSONBytes` both match expected output(s)

utils/json.go

i-norden commented

2023-01-25 02:00:39 +00:00

Let's remove this left over comment

utils/json.go

i-norden commented

2023-01-25 02:14:10 +00:00

Nice this simplifies things. In a separate PR we can simplify things further by getting rid of the linksystem and doing https://github.com/cerc-io/laconicd/issues/85.

In this context we don't need to pull in an entire linksystem, we can encode as dag-cbor using this, hash the output, and convert the hash into a CID using this.

Nice this simplifies things. In a separate PR we can simplify things further by getting rid of the linksystem and doing https://github.com/cerc-io/laconicd/issues/85. In this context we don't need to pull in an entire linksystem, we can encode as dag-cbor using [this](https://github.com/ipld/go-ipld-prime/blob/master/codec/dagcbor/marshal.go#L36), hash the output, and convert the hash into a CID using [this](https://github.com/ipld/go-ipld-prime/blob/master/linking/cid/cidLink.go#L41).

i-norden requested changes 2023-01-27 18:44:15 +00:00

i-norden left a comment

Thanks for adding the test, just one small modification.

utils/json_test.go

						
				@ -0,0 +23,4 @@

					}

					for _, tc := range testCases {

						deprecatedAndCorrect, _ := CIDFromJSONBytes([]byte(tc.content))

i-norden commented

2023-01-27 18:36:39 +00:00

We should either catch any errors returned by these functions or make sure the returned result isn't nil/empty, because if both functions were to error out and return a nil/empty result it would evaluate them as equal and the test would pass.

We should either catch any errors returned by these functions or make sure the returned result isn't `nil`/empty, because if both functions were to error out and return a `nil`/empty result it would evaluate them as equal and the test would pass.