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

[CL]: Export Position_lock_id mappings to GenesisState #4912

Merged
merged 7 commits into from
Apr 16, 2023

Conversation

stackman27
Copy link
Contributor

Closes: (#4875)[https://github.com//issues/4875]

What is the purpose of the change

Add a description of the overall background and high level changes that this PR introduces

(E.g.: This pull request improves documation of area A by adding ....

Brief Changelog

(for example:)

  • The metadata is stored in the blob store on job creation time as a persistent artifact
  • Deployments RPC transmits only the blob storage reference
  • Daemons retrieve the RPC data from the blob cache

Testing and Verifying

(Please pick one of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added unit test that validates ...
  • Added integration tests for end-to-end deployment with ...
  • Extended integration test for ...
  • Manually verified the change by ...

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
  • How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

@stackman27 stackman27 changed the title [CL]: Export state mappings to GenesisState [CL]: Export Position_lock_id mappings to GenesisState Apr 14, 2023
@stackman27 stackman27 added the V:state/breaking State machine breaking PR label Apr 14, 2023
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

Suggested changes. Please take a look and update the PR description

proto/osmosis/concentrated-liquidity/genesis.proto Outdated Show resolved Hide resolved
@stackman27 stackman27 requested a review from p0mvn April 14, 2023 21:12
Comment on lines +51 to +63
message PositionData {
Position position = 1;
uint64 lock_id = 2 [ (gogoproto.moretags) = "yaml:\"lock_id\"" ];
}

// GenesisState defines the concentrated liquidity module's genesis state.
message GenesisState {
// params are all the parameters of the module
Params params = 1 [ (gogoproto.nullable) = false ];
// pool data containining serialized pool struct and ticks.
repeated PoolData pool_data = 2 [ (gogoproto.nullable) = false ];

repeated Position positions = 3 [ (gogoproto.nullable) = false ];
repeated PositionData position_data = 3 [ (gogoproto.nullable) = false ];
Copy link
Member

@czarcas7ic czarcas7ic Apr 15, 2023

Choose a reason for hiding this comment

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

So, it seems like the only reason we need to do this is because lock_id is the only externally linked data in all of the KVStore pairs. Maybe it makes sense for us to include underlying lock ID in the position struct after all...

Thoughts @p0mvn ? Just feels odd to make a new proto that links these two when it could just exist in the position struct itself. Not a huge deal tho

Copy link
Member

Choose a reason for hiding this comment

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

Why did we remove it from the second index in the first place? Was it the fact that some position IDs don't have a lock id associated with them? If that's the case, I think the current approach is better.

Copy link
Member

Choose a reason for hiding this comment

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

I removed it from the denom because of the complexity it introduced when attempting to create intermediary accounts with the base denom rather than the full denom (it would try and gather all locks with the base denom rather the full one). Also, removing it reduced much of the custom lock logic that was added previously

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will proceed with current approach and we can change anything that comes up in the future

Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

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

On glance the logic feels right, would like to hear what Roman has to say on moving lock to position struct before approving though. Nice work!

Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

Nice work

x/concentrated-liquidity/genesis.go Outdated Show resolved Hide resolved
@czarcas7ic czarcas7ic merged commit 4f6f95f into main Apr 16, 2023
@czarcas7ic czarcas7ic deleted the sis/genesis-map branch April 16, 2023 01:15
@github-actions github-actions bot mentioned this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants