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

feat: Add ORM package DO NOT MERGE! #9491

Closed
wants to merge 16 commits into from
Closed

Conversation

likhita-809
Copy link
Contributor

ref: #9237

@github-actions github-actions bot added the T:Docs Changes and features related to documentation. label Jun 10, 2021
@likhita-809 likhita-809 changed the title Add ORM package feat: Add ORM package Jun 10, 2021
@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #9491 (3c2c426) into master (38d6702) will increase coverage by 0.31%.
The diff coverage is 82.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9491      +/-   ##
==========================================
+ Coverage   60.58%   60.89%   +0.31%     
==========================================
  Files         589      601      +12     
  Lines       37218    37769     +551     
==========================================
+ Hits        22548    23001     +453     
- Misses      12726    12787      +61     
- Partials     1944     1981      +37     
Impacted Files Coverage Δ
orm/testsupport.go 48.57% <48.57%> (ø)
orm/genesis.go 50.00% <50.00%> (ø)
orm/auto_uint64.go 76.00% <76.00%> (ø)
orm/iterator.go 80.16% <80.16%> (ø)
orm/sequence.go 81.48% <81.48%> (ø)
orm/table.go 82.29% <82.29%> (ø)
orm/indexer.go 91.04% <91.04%> (ø)
orm/index.go 91.54% <91.54%> (ø)
orm/index_key_codec.go 100.00% <100.00%> (ø)
orm/orm.go 100.00% <100.00%> (ø)
... and 14 more

@likhita-809 likhita-809 marked this pull request as ready for review June 10, 2021 13:46
@robert-zaremba
Copy link
Collaborator

This can only be merged after RC candidate is out.

@robert-zaremba robert-zaremba changed the title feat: Add ORM package feat: Add ORM package DO NOT MERGE! Jun 11, 2021
@robert-zaremba robert-zaremba added the S: DO NOT MERGE Status: DO NOT MERGE label Jun 11, 2021
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @likhita-809. This PR is pretty large. How do envision reviewers reviewing this PR effectively? Also, why are we adding an ORM to the SDK to begin with?

@likhita-809
Copy link
Contributor Author

Thanks for the contribution @likhita-809. This PR is pretty large. How do envision reviewers reviewing this PR effectively? Also, why are we adding an ORM to the SDK to begin with?

This PR is just about migrating the ORM package from regen-ledger to cosmos-sdk.
As you can see in this discussion #9156 , this PR adds a new table-store (ORM) package which allows for automatic secondary index creation for data being stored by modules.
And also there is a proposal of Group module to be merged in SDK, which will need ORM to support it

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

docs docs docs. Cant expect people to read the code to understand how to use this.

@fedekunze fedekunze marked this pull request as draft June 16, 2021 15:33
@aaronc
Copy link
Member

aaronc commented Jun 16, 2021

@ryanchristo and @blushi will be coordinating to write-up docs.

I do think it would be ideal to split this up into smaller PRs if possible. One challenge is that all of this code already did go through a few rounds of review outside the SDK when we created the group module, so I understand the motivation to pull it in all at once. But still piece by piece is probably better.

What about putting this in the store/table package? I also think it could make sense for this package to have its own go.mod so we can move towards more modularity.

@aaronc
Copy link
Member

aaronc commented Jun 16, 2021

Also, doing this piece by piece will allow us to make some upgrades to the initial version.

In particular, I'm thinking we might not want Tables and Indexes to hold direct references to StoreKeys anymore. This would allow us to declare those objects statically as vars which will be useful for #9158. Then the StoreKey would be provided together with the Context. What do you think @likhita-809 and @blushi ?

How about we start with a PR that just brings in the basic Table and Indexable functionality?

@blushi
Copy link
Contributor

blushi commented Jun 17, 2021

I believe that makes sense @aaronc

@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Aug 2, 2021
@github-actions github-actions bot closed this Aug 8, 2021
@ryanchristo ryanchristo reopened this Aug 8, 2021
@blushi
Copy link
Contributor

blushi commented Aug 9, 2021

@ryanchristo I believe we can keep this PR closed since it's just migrating the current version of the ORM package in one go and we don't wanna do that.

@blushi blushi closed this Aug 9, 2021
@tac0turtle tac0turtle deleted the likhita/add-table-store branch March 25, 2022 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: DO NOT MERGE Status: DO NOT MERGE T:Docs Changes and features related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants