feat: reverse search links #793

RiccardoM · 2022-03-23T11:45:08Z

Description

This PR adds the ability to reverse search for app links and chain links owners. The new following queries have been added:

search for app link owners using an application name (optional), and username (optional, works only if the application is specified as well)
search for chain link owners using a chain name (optional), and target address (optional, works only if the chain name is specified as well)

Closes: #777
Closes: #794

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)

…earch-app-links # Conflicts: # x/profiles/keeper/keeper_app_links.go

…earch-app-links # Conflicts: # x/profiles/keeper/migrations.go # x/profiles/legacy/v4/store_test.go

…reverse-search-app-links � Conflicts: � proto/desmos/profiles/v1beta1/models_chain_links.proto � proto/desmos/profiles/v1beta1/models_profile.proto � proto/desmos/profiles/v1beta1/models_relationships.proto � x/profiles/keeper/keeper_app_links.go � x/profiles/keeper/keeper_chain_links.go � x/profiles/keeper/keeper_chain_links_test.go � x/profiles/keeper/keeper_dtag_transfers.go � x/profiles/keeper/migrations.go � x/profiles/legacy/v4/keeper.go � x/profiles/legacy/v4/models_chain_links.pb.go � x/profiles/legacy/v4/models_profile.pb.go � x/profiles/legacy/v4/models_relationships.pb.go � x/profiles/legacy/v4/store.go � x/profiles/legacy/v4/store_test.go � x/profiles/types/query.pb.go � x/profiles/types/query_app_links.pb.go

codecov · 2022-03-23T11:49:41Z

Codecov Report

Merging #793 (3ee9064) into master (73a6816) will increase coverage by 0.03%.
The diff coverage is 84.21%.

@@            Coverage Diff             @@
##           master     #793      +/-   ##
==========================================
+ Coverage   81.25%   81.28%   +0.03%     
==========================================
  Files          76       77       +1     
  Lines        6503     6610     +107     
==========================================
+ Hits         5284     5373      +89     
- Misses        977      989      +12     
- Partials      242      248       +6

Impacted Files	Coverage Δ
x/profiles/legacy/v5/store.go	`75.51% <75.51%> (ø)`
x/profiles/keeper/grpc_query.go	`79.54% <89.65%> (+4.96%)`	⬆️
x/profiles/keeper/keeper_app_links.go	`81.94% <100.00%> (-0.25%)`	⬇️
x/profiles/keeper/keeper_chain_links.go	`84.48% <100.00%> (+0.27%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43d6ddf...3ee9064. Read the comment docs.

…reverse-search-links

…nto riccardo/reverse-search-links

x/profiles/legacy/v5/store.go

x/profiles/keeper/grpc_query.go

dadamu · 2022-03-28T04:29:05Z

x/profiles/keeper/grpc_query.go

+		cleanedKey := bytes.TrimSuffix(bytes.TrimPrefix(keyWithPrefix, types.ChainLinkChainPrefix), value)
+		values := bytes.Split(cleanedKey, types.Separator)
+		chainName, target := values[0], values[1]


I think it can be all implemented by bytes.Split instead of bytes.TrimSuffix, say:

values := bytes.Split(cleanedKey, types.Separator) chainName, target, value := values[0], values[1], values[2]

Then, the value of the key can be simply 0x01 in order to reduce the storage on chain.

Right now, links owners are stored using the following keys:

ChainLinkChainPrefix + Chain name + Separator + External Address + User address -> User address

So, if we use the bytes.Split it would return the following values:

1. Chain name 2. External Address + User address

We would have to remove the User address suffix anyway to get the external address.

Also, the key for chain link owners right now are:

ChainLinkChainPrefix = []byte{0x15} ApplicationLinkAppPrefix = []byte{0x16}

Which already are 1 byte each. We already use 0x01 for the IBCPortKey key. We can't re-use that. Also, 0x01 and 0x16 are both 1 byte each so that wouldn't impact the chain storage space

@RiccardoM Sorry for that the explanation was not clear, I mean that we can store the key like:

ChainLinkChainPrefix + Chain name + Separator + External Address + Spearator + User address -> 0x01

Then the bytes.Split would return the values:

1. Chain name 2. External address 3. User address

As a result, the storage doesn't need to store user address as value again.

@dadamu You are completely right! Thanks for pointing that out, this way we can save up a lot of disk space. I've just implemented it this way (and also added some missing tests)

Considering this answer, should we look after other places where we use bytes.TrimSuffix and bytes.TrimPrefix and see if we can improve them as well?

@bragaz All other keys instances do not trim prefixes unless necessary (eg. inside user group members). All the keys have already been reviewed inside their own PRs before being merged, and I don't think it would be appropriate to do it here anyway. I've checked though and didn't find any use of bytes.TrimPrefix aside from this case and user groups members anyway

Signed-off-by: Riccardo Montagnin <[email protected]>

dadamu · 2022-03-28T12:36:44Z

x/profiles/keeper/keeper_chain_links_test.go

@@ -355,6 +355,14 @@ func (suite *KeeperTestSuite) TestKeeper_DeleteChainLink() {
 					"cosmos",
 					ext.GetAddress().String(),
 				))
+
+				// Check the additional keys
+				store := suite.ctx.KVStore(suite.storeKey)


We should have checking the additional keys in the StoreChainlink test function as well.

dadamu · 2022-03-28T12:38:37Z

x/profiles/keeper/keeper_app_links_test.go

@@ -487,6 +487,15 @@ func (suite *KeeperTestSuite) Test_DeleteApplicationLink() {
 					"twitter",
 					"twitteruser",
 				))
+
+				// Check the additional keys
+				store := suite.ctx.KVStore(suite.storeKey)


As above, we should have the checking in the SaveApplicationLink

Signed-off-by: Riccardo Montagnin <[email protected]>

dadamu

Looks good to me, ready to go.

RiccardoM added 17 commits March 21, 2022 10:25

started working on the reverse search for app links

c2d9106

fix: fixed how chain links and app links are deleted

45ed167

chore: added changeset entry

9cdd3b0

updated how DTag transfer requests are deleted

ff758d7

removed now unused method

f65bcfa

Merge branch 'riccardo/app-chain-links-fixes' into riccardo/reverse-s…

9afd731

…earch-app-links # Conflicts: # x/profiles/keeper/keeper_app_links.go

removed now unused method

982826c

added the migration

a3a26f6

chore: renamed the v1beta1 store package to v5

90549f7

Merge branch 'riccardo/rename-legacy-package' into riccardo/reverse-s…

a1f6d38

…earch-app-links # Conflicts: # x/profiles/keeper/migrations.go # x/profiles/legacy/v4/store_test.go

run make format

76e26d8

chore: updated Proto files

ab71a63

feat: added chain link owners keys

c643093

chore: updated module version

4bc9c28

started working on chain links rever search querier

996e2f8

added gRPC tests

6ea38bc

github-actions bot added the x/profiles Module that allows to create and manage decentralized social profiles label Mar 23, 2022

Updated Swagger definition

6fea2ff

RiccardoM added 3 commits March 23, 2022 14:27

Merge branch 'master' of github.com:desmos-labs/desmos into riccardo/…

a55649a

…reverse-search-links

added changeset entry

63fedef

Merge remote-tracking branch 'origin/riccardo/reverse-search-links' i…

9c48b8d

…nto riccardo/reverse-search-links

github-actions bot added the x/CLI label Mar 23, 2022

RiccardoM requested review from dadamu and leobragaz and removed request for dadamu March 23, 2022 14:51

manu0466 mentioned this pull request Mar 24, 2022

Add support to reverse search links desmos-labs/contracts-bindings#8

Closed

dadamu reviewed Mar 25, 2022

View reviewed changes

x/profiles/legacy/v5/store.go Show resolved Hide resolved

dadamu reviewed Mar 25, 2022

View reviewed changes

x/profiles/keeper/grpc_query.go Show resolved Hide resolved

dadamu reviewed Mar 25, 2022

View reviewed changes

x/profiles/keeper/grpc_query.go Show resolved Hide resolved

added store tests

a51b8b9

leobragaz approved these changes Mar 25, 2022

View reviewed changes

dadamu reviewed Mar 28, 2022

View reviewed changes

updated how chain and app links owners are stored and added tests

ce789c3

Signed-off-by: Riccardo Montagnin <[email protected]>

dadamu reviewed Mar 28, 2022

View reviewed changes

updated test names and added missing tests

3ee9064

Signed-off-by: Riccardo Montagnin <[email protected]>

dadamu approved these changes Mar 28, 2022

View reviewed changes

RiccardoM added the automerge Automatically merge PR once all prerequisites pass label Mar 28, 2022

Merge branch 'master' into riccardo/reverse-search-links

6fd74df

mergify bot merged commit acdecc9 into master Mar 28, 2022

mergify bot deleted the riccardo/reverse-search-links branch March 28, 2022 14:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: reverse search links #793

feat: reverse search links #793

RiccardoM commented Mar 23, 2022 •

edited

Loading

codecov bot commented Mar 23, 2022 •

edited

Loading

dadamu Mar 28, 2022 •

edited

Loading

RiccardoM Mar 28, 2022 •

edited

Loading

dadamu Mar 28, 2022 •

edited

Loading

RiccardoM Mar 28, 2022

leobragaz Mar 28, 2022

RiccardoM Mar 28, 2022

dadamu Mar 28, 2022 •

edited

Loading

RiccardoM Mar 28, 2022

dadamu Mar 28, 2022

RiccardoM Mar 28, 2022

dadamu left a comment

feat: reverse search links #793

feat: reverse search links #793

Conversation

RiccardoM commented Mar 23, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Mar 23, 2022 • edited Loading

Codecov Report

dadamu Mar 28, 2022 • edited Loading

Choose a reason for hiding this comment

RiccardoM Mar 28, 2022 • edited Loading

Choose a reason for hiding this comment

dadamu Mar 28, 2022 • edited Loading

Choose a reason for hiding this comment

RiccardoM Mar 28, 2022

Choose a reason for hiding this comment

leobragaz Mar 28, 2022

Choose a reason for hiding this comment

RiccardoM Mar 28, 2022

Choose a reason for hiding this comment

dadamu Mar 28, 2022 • edited Loading

Choose a reason for hiding this comment

RiccardoM Mar 28, 2022

Choose a reason for hiding this comment

dadamu Mar 28, 2022

Choose a reason for hiding this comment

RiccardoM Mar 28, 2022

Choose a reason for hiding this comment

dadamu left a comment

Choose a reason for hiding this comment

RiccardoM commented Mar 23, 2022 •

edited

Loading

codecov bot commented Mar 23, 2022 •

edited

Loading

dadamu Mar 28, 2022 •

edited

Loading

RiccardoM Mar 28, 2022 •

edited

Loading

dadamu Mar 28, 2022 •

edited

Loading

dadamu Mar 28, 2022 •

edited

Loading