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.

Sign up for GitHub

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

Jump to bottom

[Feature][market] swap constant product #233

Merged
merged 13 commits into from
Oct 15, 2019
Merged

[Feature][market] swap constant product #233

merged 13 commits into from
Oct 15, 2019

Conversation

yun-yeo
Copy link
Contributor

@yun-yeo yun-yeo commented Sep 17, 2019 •
edited
Loading

Summary of changes

As proposed here and #226 , apply constant product to swap feature.

New EndPoint

/market/eq_terra_pool
/market/pool_info/{denom}

Report of required housekeeping

  • Github issue OR spec proposal link
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added a relevant changelog entry: clog add [section] [stanza] [message]

(FOR ADMIN) Before merging

  • Added appropriate labels to PR
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)
  • Confirm added tests are consistent with the intended behavior of changes
  • Ensure all tests pass

@codecov
Copy link

codecov bot commented Sep 17, 2019 •
edited
Loading

Codecov Report

Merging #233 into develop will increase coverage by 0.81%.
The diff coverage is 71.3%.

@@             Coverage Diff             @@
##           develop     #233      +/-   ##
===========================================
+ Coverage    67.71%   68.52%   +0.81%     
===========================================
  Files           70       71       +1     
  Lines         3026     3117      +91     
===========================================
+ Hits          2049     2136      +87     
+ Misses         869      866       -3     
- Partials       108      115       +7

@yun-yeo yun-yeo force-pushed the feature/swap-constant-product branch 2 times, most recently from 586e532 to 886d454 Compare September 18, 2019 03:15
@yun-yeo yun-yeo requested a review from dokwon September 18, 2019 03:16
@yun-yeo yun-yeo changed the title Feature/swap constant product(wip) Feature/swap constant product Sep 18, 2019
@yun-yeo yun-yeo self-assigned this Sep 18, 2019
@yun-yeo yun-yeo added enhancement New feature or request must Mustfix for target release. labels Sep 18, 2019
@yun-yeo yun-yeo force-pushed the feature/swap-constant-product branch from 80d41c4 to c72cd91 Compare September 19, 2019 07:17
@yun-yeo yun-yeo closed this Sep 19, 2019
@yun-yeo yun-yeo reopened this Sep 19, 2019
@yun-yeo yun-yeo force-pushed the feature/swap-constant-product branch 3 times, most recently from 217a346 to 0a1cea0 Compare September 20, 2019 06:29
@dokwon
Copy link
Contributor

dokwon commented Sep 23, 2019

Codecov is down; can we do something about that?

dokwon
dokwon suggested changes Sep 23, 2019
View reviewed changes
Copy link
Contributor

@dokwon dokwon left a comment

Choose a reason for hiding this comment

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

Mostly stylistic changes; consider making comments, function decomposition more informative and intuitive.

x/market/internal/keeper/oracle_hooks.go Outdated

// AfterValidatorBonded register voting info for a validator
func (k Keeper) AfterPriceUpdated(ctx sdk.Context, denom string, price sdk.Dec) {
if denom == core.MicroSDRDenom {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment as to why SDR has an exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

x/market/internal/keeper/oracle_hooks.go Outdated
)

// AfterValidatorBonded register voting info for a validator
func (k Keeper) AfterPriceUpdated(ctx sdk.Context, denom string, price sdk.Dec) {
Copy link
Contributor

Choose a reason for hiding this comment

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

AfterPriceUpdated --> AfterPriceUpdate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

x/market/internal/keeper/oracle_hooks.go Outdated
"github.com/terra-project/core/x/market/internal/types"
)

// AfterValidatorBonded register voting info for a validator
Copy link
Contributor

Choose a reason for hiding this comment

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

Match Comment to function name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

x/market/internal/keeper/keeper_test.go Outdated
}

func TestGetDecSwapCoin(t *testing.T) {
func TestApplyDeltaRegression(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Regression? Add comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

x/market/internal/keeper/oracle_hooks.go Outdated
return Hooks{k}
}

// Implements StakingHooks
Copy link
Contributor

Choose a reason for hiding this comment

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

AfterPriceUpdate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

x/market/internal/keeper/swap.go
return
}

// ComputeInternalSwap returns the amount of asked DecCoin should be returned for a given offerCoin at the effective
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessary, but for convenience.
In swap process and treasury module, ComputeInternalSwap is used to convert a denom to another at the effective exchange rate without spread.

x/market/internal/types/pool_info.go Outdated
sdk "github.com/cosmos/cosmos-sdk/types"
)

// PoolInfo defines information to describe each denomination pool
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment is not informative; "describe each denomination pool" to describe an "info pool" is not sufficiently descriptive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

x/market/internal/types/pool_info.go Outdated
RegressionAmount: regressionAmount,
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Add // Extends fmt.Stringer comment to help the linter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

x/oracle/abci.go Outdated
@@ -56,6 +56,8 @@ func EndBlocker(ctx sdk.Context, k Keeper) {
// TODO - update tax-cap
// Set price to the store
k.SetLunaPrice(ctx, denom, mod)
k.AfterPriceUpdated(ctx, denom, mod)
Copy link
Contributor

Choose a reason for hiding this comment

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

Name of function could be improved; perhaps "PriceUpdateCleanup", or consider decomposing the cleanup functions into identifiable chunks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

x/oracle/internal/types/expected_keeper.go Outdated
@@ -33,3 +33,8 @@ type SupplyKeeper interface {
type StakingHooks interface {
AfterValidatorBonded(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) // Must be called when a validator is bonded
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need oracle hooks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To keep oracle swap rate for safe base pool update, but removed. It will retry until success when there is no oracle price.

@dokwon
Copy link
Contributor

dokwon commented Sep 23, 2019

Ignoring codecov warning for now

@yun-yeo yun-yeo force-pushed the feature/swap-constant-product branch 2 times, most recently from 2f8b877 to 4e6ba3d Compare September 23, 2019 10:30
@yun-yeo yun-yeo requested a review from dokwon September 23, 2019 10:31
@yun-yeo yun-yeo force-pushed the feature/swap-constant-product branch from 4e6ba3d to 1272bd6 Compare September 24, 2019 02:31
@yun-yeo yun-yeo force-pushed the feature/swap-constant-product branch from 2f951ed to d4e6dc5 Compare September 24, 2019 09:40
@yun-yeo yun-yeo force-pushed the feature/swap-constant-product branch from d4e6dc5 to e3fb247 Compare September 24, 2019 09:58
@yun-yeo yun-yeo force-pushed the feature/swap-constant-product branch from 1a13bf8 to 037bb91 Compare October 14, 2019 09:18
@yun-yeo yun-yeo changed the title Feature/swap constant product [Feature][market] swap constant product Oct 14, 2019
dokwon
dokwon approved these changes Oct 15, 2019
View reviewed changes
client/lcd/swagger-ui/swagger.yaml Outdated
get:
summary: Get prev day issuance
summary: Get TERRA pool amount
Copy link
Contributor

Choose a reason for hiding this comment

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

Get the current size of the Terra swap pool, denominated in usdr

client/lcd/swagger-ui/swagger.yaml Outdated
@@ -1497,9 +1497,9 @@ paths:
$ref: "#/definitions/Coin"
500:
description: Internal Server Error
/market/prev_day_issuance:
/market/terra_pool:
Copy link
Contributor

Choose a reason for hiding this comment

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

terra_pool --> terra_swap_pool

client/lcd/swagger-ui/swagger.yaml Outdated
description: Internal Server Error
/market/luna_pool:
get:
summary: Get LUNA pool amount
Copy link
Contributor

Choose a reason for hiding this comment

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

Get the current size of the Luna swap pool, denominated in uluna

client/lcd/swagger-ui/swagger.yaml Outdated
description: Internal Server Error
/market/last_update_height:
get:
summary: Get last pool update height
Copy link
Contributor

Choose a reason for hiding this comment

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

Get the last swap pool reset height

client/lcd/swagger-ui/swagger.yaml Outdated
description: Bad Request
500:
description: Internal Server Error
/market/last_update_height:
Copy link
Contributor

Choose a reason for hiding this comment

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

last_cp_update_height

@dokwon dokwon merged commit 4f9535d into develop Oct 15, 2019
@dokwon dokwon deleted the feature/swap-constant-product branch October 15, 2019 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dokwon dokwon dokwon approved these changes

Assignees

@yun-yeo yun-yeo

Labels
enhancement New feature or request must Mustfix for target release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yun-yeo @dokwon