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: liquidity module #2423

Closed
wants to merge 34 commits into from
Closed

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Apr 20, 2023

Description

  • add liquidity module for future removal
  • add the liquidity module to the cosmos hub repo

This PR is aimed at making the process of bringing the liqjuidity module into the cosmos hub repository easier, which in turn will make the eventual removal of the liquidity module easier.

Reasoning: currently it is difficult to make changes to the liqudity module. As far as I know, B-Harvest is no longer being paid to work on this. I think that Gaia should solve her own problems.


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...

  • Included the correct type prefix in the PR title
  • Added ! to the type prefix if API or client breaking change
  • Targeted the correct branch (see PR Targeting)
  • Provided a link to the relevant issue or specification
  • Followed the guidelines for building SDK modules
  • Included the necessary unit and integration tests
  • Added a changelog entry to CHANGELOG.md
  • Included comments for documenting Go code
  • Updated the relevant documentation or specification
  • Reviewed "Files changed" and left comments if necessary
  • Confirmed all CI checks have passed

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

@faddat faddat changed the title feat: liquidity mmodule feat: liquidity module Apr 20, 2023
@faddat faddat marked this pull request as ready for review May 8, 2023 06:37
@mpoke
Copy link
Contributor

mpoke commented May 8, 2023

@faddat Thank you for your contribution. Please note that there are already two open issues on this topic: this EPIC #2084 and this issue #2346. Please participate in discussions with the other contributors before opening PRs.

@faddat
Copy link
Contributor Author

faddat commented May 9, 2023

@mpoke I take issue with the plan. I don't think it is realistic, and I think that it amounts to slashing users.

I think that anyone should open a PR to any cosmos repo that they'd like to, for any reason, and that contributions should not be resented.

Do you have any proof that the path unwinding needed to affect the change described in #2084 and #2346 without harming user funds and harming trust in the hub is possible in the next 6 months, given that counterparty chains would need to assent?

Why are you / informal systems promoting a course of action that will by necessity result in the loss of user funds?

@faddat faddat marked this pull request as draft May 9, 2023 11:40
@mpoke
Copy link
Contributor

mpoke commented May 9, 2023

I take issue with the plan. I don't think it is realistic, and I think that it amounts to slashing users.

@faddat Thank you for your feedback. Please share it on the opened issues (i.e., #2084, #2346) and participate in the discussions.

I think that anyone should open a PR to any cosmos repo that they'd like to, for any reason, and that contributions should not be resented.

We encourage and appreciate all contributions to this repo. To ensure a smooth workflow for all contributors, a general procedure for contributing has been established. Please respect the other contributors and their work.

@faddat
Copy link
Contributor Author

faddat commented May 9, 2023

@mpoke please let me know what I've said or done that's disrespectful.

I mean there was that one time I gave your CEO Bucky double middle fingers while on the Don Cryptonium show, but I very hope he knows I no longer feel that way about him, and that was more than a year ago now, and I imagine I had some choice words and it was likely disrespectful. But sir, what about this PR is disrespectful?

It is my strong opinion that you're framing this contribution itself as disrespectful, and I respectfully disagree with that stance.

Since you seem inclined to reject this pull request, I'd like evidence that the plans in those issues are viable.

I'm claiming:

  1. there is little to no chance that the path unwinding option is viable on any reasonable timeline.
  2. simply removing the liquidty module from the hub is no different from a slashing proposal at this time

....therefore it makes sese to not damage trust, and to keep the module.

I also have a practical problem:

https://github.com/cosmos/gaia/actions/runs/4931156061/jobs/8812901629?pr=2423

any idea what is going on there with the grpc queries?

faddat and others added 2 commits May 10, 2023 05:42
@faddat faddat marked this pull request as ready for review May 10, 2023 08:42
@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Merging #2423 (ff31716) into main (a0cd727) will increase coverage by 1.12%.
The diff coverage is 83.31%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2423      +/-   ##
==========================================
+ Coverage   84.99%   86.11%   +1.12%     
==========================================
  Files          22       46      +24     
  Lines        1599     4796    +3197     
==========================================
+ Hits         1359     4130    +2771     
- Misses        193      465     +272     
- Partials       47      201     +154     
Impacted Files Coverage Δ
app/keepers/keepers.go 98.91% <ø> (ø)
app/keepers/keys.go 86.20% <ø> (ø)
app/modules.go 100.00% <ø> (ø)
x/liquidity/types/liquidity_pool.go 98.11% <ø> (ø)
x/liquidity/types/msgs.go 100.00% <ø> (ø)
x/liquidity/types/params.go 98.34% <ø> (ø)
x/liquidity/types/swap.go 90.71% <ø> (ø)
x/liquidity/types/utils.go 93.58% <ø> (ø)
x/liquidity/keeper/migration.go 50.00% <50.00%> (ø)
x/liquidity/keeper/swap.go 65.83% <65.83%> (ø)
... and 17 more

... and 1 file with indirect coverage changes

@jtremback
Copy link
Contributor

@faddat See my comment here.

As far as I can tell, @dongsam's migration will return all but $1000 of the pool balances to their owner's accounts. Correct me if I'm wrong.

I don't think it's worth bringing a zombie module in the codebase for $1000 instead of just transferring to the community pool and sorting it out from there.

This will be going to governance first in any case.

@faddat
Copy link
Contributor Author

faddat commented May 11, 2023

@jtremback much, much better than a stick in the eye sir.

(or a dead module in the codebase)

please expect my yeswithveto.

Since you @jtremback and @mpoke work for the same organization in Cosmos, maybe you can tell me what he found offensive about this pull request?

He's making some pretty bold claims here and I'm confused, officially confused, as a cosmos hub contributor.

@faddat
Copy link
Contributor Author

faddat commented May 13, 2023

Another thing I'd like to add @jtremback -- I notice that your comment is from two days ago.

What was @mpoke referring to? Sorry to keep bringing this up but it is clearly relevant to the code, to him, anyhow. It seems he feels I had disrespected other contributors and that is worrisome to me.

Marius' comment, as well as this PR, are much more than two days old. So, maybe @mpoke can please clarify what the issue was?

@faddat faddat closed this May 17, 2023
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.

4 participants