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 special asset pairs #2150

Merged
merged 47 commits into from
Aug 1, 2023
Merged

feat: add special asset pairs #2150

merged 47 commits into from
Aug 1, 2023

Conversation

toteki
Copy link
Member

@toteki toteki commented Jul 17, 2023

Contains logic for creating and storing these pairs, but not their actual effects on borrow limit.

These can be released early to accelerate their proposal timeline.

@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Merging #2150 (5e2e948) into main (7f05ad4) will decrease coverage by 6.52%.
Report is 160 commits behind head on main.
The diff coverage is 82.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2150      +/-   ##
==========================================
- Coverage   75.38%   68.86%   -6.52%     
==========================================
  Files         100      151      +51     
  Lines        8025    11680    +3655     
==========================================
+ Hits         6050     8044    +1994     
- Misses       1589     3095    +1506     
- Partials      386      541     +155     
Files Changed Coverage Δ
ante/spam_prevention.go 75.92% <ø> (ø)
x/incentive/codec.go 47.82% <ø> (+9.89%) ⬆️
x/incentive/keeper/invariants.go 0.00% <0.00%> (ø)
x/incentive/keeper/store.go 70.67% <ø> (ø)
x/incentive/keeper/unbonding.go 80.45% <ø> (ø)
x/incentive/keeper/update.go 52.11% <ø> (ø)
x/incentive/keys.go 100.00% <ø> (ø)
x/incentive/msgs.go 77.41% <ø> (-0.81%) ⬇️
x/incentive/params.go 89.28% <ø> (-10.72%) ⬇️
x/leverage/client/tests/suite.go 100.00% <ø> (ø)
... and 55 more

... and 56 files with indirect coverage changes

@toteki toteki marked this pull request as ready for review July 20, 2023 04:10
@toteki toteki requested a review from a team as a code owner July 20, 2023 04:10
Copy link
Member

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

  • need to remove title and description from gov messages
  • let's talk about using list of assets rather than a pair.
  • use store helpers

proto/umee/leverage/v1/leverage.proto Outdated Show resolved Hide resolved
proto/umee/leverage/v1/leverage.proto Outdated Show resolved Hide resolved
proto/umee/leverage/v1/query.proto Outdated Show resolved Hide resolved
proto/umee/leverage/v1/tx.proto Outdated Show resolved Hide resolved
proto/umee/leverage/v1/leverage.proto Show resolved Hide resolved
x/leverage/keeper/token.go Outdated Show resolved Hide resolved
x/leverage/types/errors.go Outdated Show resolved Hide resolved
x/leverage/keeper/token.go Outdated Show resolved Hide resolved
x/leverage/types/keys.go Show resolved Hide resolved
x/leverage/types/msgs.go Outdated Show resolved Hide resolved
toteki and others added 8 commits July 21, 2023 14:06
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
x/leverage/types/proposal.go Outdated Show resolved Hide resolved
@toteki toteki requested a review from a team as a code owner July 26, 2023 14:44
Copy link
Contributor

@kosegor kosegor left a comment

Choose a reason for hiding this comment

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

Overall looks good, adding some minor comments.

proto/umee/leverage/v1/leverage.proto Outdated Show resolved Hide resolved
proto/umee/leverage/v1/tx.proto Outdated Show resolved Hide resolved
proto/umee/leverage/v1/tx.proto Outdated Show resolved Hide resolved
x/leverage/client/cli/query.go Outdated Show resolved Hide resolved
x/leverage/types/genesis.go Outdated Show resolved Hide resolved
proto/umee/leverage/v1/leverage.proto Show resolved Hide resolved
x/leverage/types/token.go Outdated Show resolved Hide resolved
Copy link
Member

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

  • Instead of simplifying user interface by using sets, this iteration added additional complexity to add both pairs and sets. Moreover the the pars are stored in each direction. So for a set (A,B,C) we store 6 pairs (and return 6 pairs when querying it, instead of 1): (A,B), (A,C), (B,C), (B,A), (B,C), (C,B)` -- that is against what we wanted based on the discussion in Slack.
  • if under the hood we save sets as pairs, then we need to rethink the key structure and assure we don't have duplicates.
  • let's don't create new error codes, which are already deprecated.
  • prefer to not use yaml
  • let's make sure we have unit-tests task in the Linear.

proto/umee/leverage/v1/query.proto Outdated Show resolved Hide resolved
proto/umee/leverage/v1/query.proto Outdated Show resolved Hide resolved
proto/umee/leverage/v1/tx.proto Outdated Show resolved Hide resolved
proto/umee/leverage/v1/tx.proto Show resolved Hide resolved
x/leverage/client/cli/query.go Outdated Show resolved Hide resolved
x/leverage/types/msgs.go Outdated Show resolved Hide resolved
x/leverage/types/msgs.go Show resolved Hide resolved
x/leverage/types/msgs.go Show resolved Hide resolved
x/leverage/types/proposal.go Outdated Show resolved Hide resolved
x/leverage/types/token.go Outdated Show resolved Hide resolved
@toteki toteki dismissed robert-zaremba’s stale review August 1, 2023 03:40

merging this as discussed

@toteki toteki added this pull request to the merge queue Aug 1, 2023
Merged via the queue into main with commit ea52e22 Aug 1, 2023
27 checks passed
@toteki toteki deleted the adam/special branch August 1, 2023 03:41
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.

3 participants