Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get Contracts by Creator Address #1021

Merged
merged 37 commits into from
Oct 20, 2022
Merged

Conversation

vuong177
Copy link
Contributor

@vuong177 vuong177 commented Sep 23, 2022

Closes : #998

What in this PR:

  • Add a new index for getting contracts by creator address.

  • Create a migration to build the initial index.

x/wasm/keeper/keeper.go Fixed Show fixed Hide fixed
@jhernandezb
Copy link
Contributor

We need to provide a migration for this so that chains upgrading can build the initial index.

@codecov
Copy link

codecov bot commented Sep 25, 2022

Codecov Report

Merging #1021 (6828aa2) into main (9858034) will decrease coverage by 0.03%.
The diff coverage is 57.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1021      +/-   ##
==========================================
- Coverage   59.29%   59.25%   -0.04%     
==========================================
  Files          52       53       +1     
  Lines        6630     6718      +88     
==========================================
+ Hits         3931     3981      +50     
- Misses       2408     2442      +34     
- Partials      291      295       +4     
Impacted Files Coverage Δ
x/wasm/client/cli/query.go 0.00% <0.00%> (ø)
x/wasm/module.go 37.90% <60.00%> (+0.40%) ⬆️
x/wasm/keeper/keeper.go 87.40% <71.42%> (-0.36%) ⬇️
x/wasm/keeper/querier.go 72.32% <90.00%> (+1.73%) ⬆️
x/wasm/keeper/migrations.go 100.00% <100.00%> (ø)
x/wasm/types/keys.go 60.71% <100.00%> (+9.60%) ⬆️

@vuong177
Copy link
Contributor Author

vuong177 commented Sep 25, 2022

We need to provide a migration for this so that chains upgrading can build the initial index.

I'm going to make this. Thanks for your suggestion.
.

@vuong177 vuong177 marked this pull request as ready for review September 25, 2022 10:50
@vuong177 vuong177 requested a review from alpe as a code owner September 25, 2022 10:50
Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for driving this forward. 👏 🏄
It looks very good already! Just some minor comments that should be easy to address!

x/wasm/client/cli/query.go Outdated Show resolved Hide resolved
x/wasm/client/cli/query.go Show resolved Hide resolved
x/wasm/keeper/genesis_test.go Outdated Show resolved Hide resolved
x/wasm/keeper/genesis_test.go Outdated Show resolved Hide resolved
x/wasm/keeper/keeper.go Outdated Show resolved Hide resolved
x/wasm/module.go Outdated Show resolved Hide resolved
x/wasm/types/exported_keepers.go Show resolved Hide resolved
x/wasm/types/keys.go Outdated Show resolved Hide resolved
x/wasm/types/keys.go Outdated Show resolved Hide resolved
x/wasm/types/keys_test.go Show resolved Hide resolved
x/wasm/keeper/keeper.go Outdated Show resolved Hide resolved
m.keeper.addToContractCreatorThirdIndex(ctx, creator, contractAddress)
}
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test for the migration is required.

vuong177 and others added 6 commits September 27, 2022 18:12
Co-authored-by: Alexander Peters <alpe@users.noreply.github.com>
Co-authored-by: Alexander Peters <alpe@users.noreply.github.com>
@vuong177
Copy link
Contributor Author

@alpe thanks for the clear suggestions. I'm making it

Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good progress and tests. Thanks for your contribution! 💐
Still some changes are mandatory. No rush. I would let you handle this but please let me know if you don't have enough capacity or want somebody to jump in.

x/wasm/keeper/keeper.go Outdated Show resolved Hide resolved
x/wasm/keeper/keeper.go Outdated Show resolved Hide resolved
x/wasm/keeper/keeper_test.go Show resolved Hide resolved
x/wasm/keeper/keeper_test.go Outdated Show resolved Hide resolved
x/wasm/keeper/keeper_test.go Outdated Show resolved Hide resolved
x/wasm/keeper/keeper_test.go Show resolved Hide resolved
x/wasm/keeper/migrations.go Outdated Show resolved Hide resolved
@vuong177
Copy link
Contributor Author

I made this PR to draft and will be ready when I complete your suggested change. Thank you again, @alpe, I learned many things from your reviews.

@vuong177 vuong177 marked this pull request as draft September 28, 2022 08:25
@alpe alpe added this to the v0.30.0 milestone Sep 28, 2022
x/wasm/keeper/querier.go Fixed Show fixed Hide fixed
@@ -51,7 +51,8 @@ func GetContractAddressKey(addr sdk.AccAddress) []byte {

// GetContractsByCreatorPrefix returns the contracts by creator prefix for the WASM contract instance
func GetContractsByCreatorPrefix(addr sdk.AccAddress) []byte {
return append(ContractsByCreatorPrefix, addr...)
bz := address.MustLengthPrefix(addr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thanks!

@vuong177 vuong177 marked this pull request as ready for review September 30, 2022 10:32
@alpe alpe merged commit 6d67d5b into CosmWasm:main Oct 20, 2022
@alpe
Copy link
Contributor

alpe commented Oct 20, 2022

Thanks a log for your work on this. This was really helping. 💯

NoahSaso pushed a commit to NoahSaso/wasmd that referenced this pull request Dec 2, 2022
* add query to query.proto

* add ContractsByCreatorPrefix in keys.go

* add ContractCreatorThirdIndex to keeper.go

* add querier

* cli

* fix test

* linting

* add key test

* no need to change creator when migrate

* add query test

* minor

* add migrate logic

* add more test

* register migration

* minor

* Update x/wasm/client/cli/query.go

Co-authored-by: Alexander Peters <alpe@users.noreply.github.com>

* nits

* remove IterateAllContract

* Update x/wasm/keeper/genesis_test.go

Co-authored-by: Alexander Peters <alpe@users.noreply.github.com>

* nit

* nit: func name

* change key

* improve TestIteratorContractByCreator

* fix test

* use IterateContractInfo in migrate2to3

* minor

* move key

* improve test case

* add pagReq in ContractsByCreator query

* ordering query

* add migrate test

* Make ContractsByCreator plural; formatting and minor updates

* Comment why AbsoluteTxPositionLen makes sense

* Migrate 1 to 2

* Set module version

Co-authored-by: Alexander Peters <alpe@users.noreply.github.com>
Co-authored-by: khanh-notional <50263489+catShaark@users.noreply.github.com>
Magicloud pushed a commit to fpco/wasmd that referenced this pull request Jan 13, 2023
* add query to query.proto

* add ContractsByCreatorPrefix in keys.go

* add ContractCreatorThirdIndex to keeper.go

* add querier

* cli

* fix test

* linting

* add key test

* no need to change creator when migrate

* add query test

* minor

* add migrate logic

* add more test

* register migration

* minor

* Update x/wasm/client/cli/query.go

Co-authored-by: Alexander Peters <alpe@users.noreply.github.com>

* nits

* remove IterateAllContract

* Update x/wasm/keeper/genesis_test.go

Co-authored-by: Alexander Peters <alpe@users.noreply.github.com>

* nit

* nit: func name

* change key

* improve TestIteratorContractByCreator

* fix test

* use IterateContractInfo in migrate2to3

* minor

* move key

* improve test case

* add pagReq in ContractsByCreator query

* ordering query

* add migrate test

* Make ContractsByCreator plural; formatting and minor updates

* Comment why AbsoluteTxPositionLen makes sense

* Migrate 1 to 2

* Set module version

Co-authored-by: Alexander Peters <alpe@users.noreply.github.com>
Co-authored-by: khanh-notional <50263489+catShaark@users.noreply.github.com>
conorpp pushed a commit to wormhole-foundation/wasmd that referenced this pull request Feb 1, 2023
* add query to query.proto

* add ContractsByCreatorPrefix in keys.go

* add ContractCreatorThirdIndex to keeper.go

* add querier

* cli

* fix test

* linting

* add key test

* no need to change creator when migrate

* add query test

* minor

* add migrate logic

* add more test

* register migration

* minor

* Update x/wasm/client/cli/query.go

Co-authored-by: Alexander Peters <alpe@users.noreply.github.com>

* nits

* remove IterateAllContract

* Update x/wasm/keeper/genesis_test.go

Co-authored-by: Alexander Peters <alpe@users.noreply.github.com>

* nit

* nit: func name

* change key

* improve TestIteratorContractByCreator

* fix test

* use IterateContractInfo in migrate2to3

* minor

* move key

* improve test case

* add pagReq in ContractsByCreator query

* ordering query

* add migrate test

* Make ContractsByCreator plural; formatting and minor updates

* Comment why AbsoluteTxPositionLen makes sense

* Migrate 1 to 2

* Set module version

Co-authored-by: Alexander Peters <alpe@users.noreply.github.com>
Co-authored-by: khanh-notional <50263489+catShaark@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get Contracts by Creator Address
4 participants