WIP: Uniswap Module #4518

colin-axner · 2019-06-07T23:45:38Z

Addresses #4443

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added a relevant changelog entry: clog add [section] [stanza] [message]
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

…lin/4443-uniswap

x/uniswap/handler.go

fedekunze

very excited about this module. Added some early comments. Check this issue for the next upcoming standard format for modules #4438

x/uniswap/genesis.go

x/uniswap/keeper/expected_keepers.go

x/uniswap/module.go

x/uniswap/keeper/keeper.go

x/uniswap/types/msgs.go

x/uniswap/internal/keeper/keeper.go

x/uniswap/internal/types/msgs.go

x/uniswap/internal/types/errors.go

x/uniswap/handler.go

x/uniswap/internal/keeper/keeper.go

AdityaSripal

WIP review

x/uniswap/internal/types/msgs.go

AdityaSripal · 2019-06-14T19:07:49Z

x/uniswap/internal/types/params.go

+)
+
+// uniswap parameters
+// Fee = 1 - (FeeN - FeeD)


Do you mean FeeN / FeeD

yea, will update

AdityaSripal · 2019-06-14T19:11:44Z

x/uniswap/internal/types/params.go

+// uniswap parameters
+// Fee = 1 - (FeeN - FeeD)
+type FeeParams struct {
+	FeeN sdk.Int `json:"fee_numerator"`   // fee numerator


Why don't you just use sdk.Dec here?

The way getInputAmont/getOutputAmount are constructed in keeper.go requires these params. Would love help on reworking the equations to use the fee as sdk.Dec

Agree, let’s use Dec

AdityaSripal · 2019-06-14T20:37:51Z

x/uniswap/handler.go

+// HandleMsgSwapOrder handler for MsgSwapOrder
+func HandleMsgSwapOrder(ctx sdk.Context, msg MsgSwapOrder, k Keeper) sdk.Result {
+	if msg.IsBuyOrder {
+		inputAmount := k.GetInputAmount(ctx, msg.SwapDenom, msg.Amount)


Where does the bound check occur?? Shouldn't it be added after this line?

Also a deadline check is needed right?

yea, also need to check that sender has enough of coin being sold. Also need checks balance checks for adding/removing liquidity

aayushijain23 · 2019-06-14T22:48:23Z

x/uniswap/internal/keeper/querier.go

+			return queryBalance(ctx, req, k)
+		case types.QueryLiquidity:
+			return queryLiquidity(ctx, req, k)
+		case types.Parameters:


I think this is a typo. Should be QueryParameters instead. Similarly, I'm guessing the method to be called is queryParameters()

aayushijain23 · 2019-06-17T19:08:29Z

x/uniswap/internal/types/codec.go

+	ModuleCdc = codec.New()
+	RegisterCodec(ModuleCdc)
+	ModuleCdc.Seal()
+}


I guess codec.New() needs to be called once.

aayushijain23 · 2019-06-18T20:02:55Z

x/uniswap/handler.go

+
+	return sdk.Result{}
+}
+


I was going through 'GetInputPriceandGetOutputPrice` mentioned in https://github.com/runtimeverification/verified-smart-contracts/blob/uniswap/uniswap/x-y-k.pdf

After discussing with @AdityaSripal we both feel that you swapped lesser than and greater than when it comes to buy order and sell order.

For instance, for a buy order, the calculated amount should be lesser than the maximum amount the sender is willing to pay. Thus change if msg.Input.Amount.LT(calculatedAmount) to if calculatedAmount.LT(msg.input.Amount)

Similarly for a sell order, the calculated amount should be greater than the minimum amount the sender is willing to pay. Thus change if msg.Output.Amount.GT(outputAmount) to if calculatedAmount.GT(msg.Output.Amount). I would change outputAmount to calculatedAmount for uniformity.

A better way to write the comment would be to specify calculated amount first followed by the amount the sender is willing to pay.

aayushijain23 · 2019-06-18T20:04:56Z

x/uniswap/internal/keeper/keeper.go

+		panic(err)
+	}
+	return
+}


I guess it's better to use MustUnmarshalBinaryBare and MustMarshalBinaryBare instead. They take care of the panic part. This way the code gets reduced to one line and we don't even require the two methods.

aayushijain23 · 2019-06-18T20:40:49Z

x/uniswap/handler.go

+		k.CreateReservePool(ctx, msg.Denom)
+	}
+
+	nativeLiqudiity, err := k.GetReservePool(ctx, NativeAsset)


Just a minor typo.

…lin/4443-uniswap

golangcibot · 2019-06-21T00:19:27Z

x/uniswap/internal/keeper/querier.go

+	return bz, nil
+}
+
+// queryLiquidity returns the total liquidity avaliable for the provided denomination


avaliable is a misspelling of available (from misspell)

colin-axner · 2019-06-21T00:22:54Z

x/uniswap/internal/types/params.go

+// ValidateParams validates a set of params
+func ValidateParams(p Params) error {
+	// TODO: ensure equivalent sdk.validateDenom validation
+	if strings.TrimSpace(p.NativeDenom) != "" {


I'm not sure how to go about validating this since validateDenom is private. Maybe just call sdk.NewCoin(p.NativeDenom, sdk.ZeroInt()) to see if it panics and then catch the error?

Maybe just make ValidateDenom public?

colin-axner · 2019-06-22T00:57:10Z

x/uniswap/internal/keeper/keeper.go

+}
+
+// HasReservePool returns true if the reserve pool exists
+func (keeper Keeper) HasReservePool(ctx sdk.Context, denom string) bool {


Gonna remove this and add a boolean to GetReservePool to indicate if the reserve pool exists or not

fedekunze · 2019-06-25T12:37:58Z

cam we update the module based on #4618? 🙏

colin-axner · 2019-06-28T22:27:13Z

closing to switch from fork as base to branch on this repo, see #4644 for future development

colin-axner added 7 commits May 30, 2019 15:57

init scaffolding

b6aceac

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into co…

fe60ff4

…lin/4443-uniswap

base files added for uniswap module

d2b7ba5

modifiy module layout to follow standard

7609e05

added tests for msgs

9e56eee

fixes in keeper

43f29f0

work continued on keeper design

930f5ed

golangcibot reviewed Jun 7, 2019

View reviewed changes

x/uniswap/handler.go Outdated Show resolved Hide resolved

fedekunze reviewed Jun 8, 2019

View reviewed changes

added fee params, reflected pr comments

c04b239

golangcibot reviewed Jun 10, 2019

View reviewed changes

x/uniswap/internal/keeper/keeper.go Outdated Show resolved Hide resolved

colin-axner added 2 commits June 11, 2019 10:49

updated keeper

91da176

updating handler

a8437b0

fedekunze reviewed Jun 12, 2019

View reviewed changes

added querier and basic querier test

8796822

AdityaSripal requested changes Jun 14, 2019

View reviewed changes

update msgs fields to input/output

5e366fd

aayushijain23 reviewed Jun 14, 2019

View reviewed changes

rename exchange to reserve pool, updated handler with err checks

7cfb8c1

aayushijain23 reviewed Jun 17, 2019

View reviewed changes

updated params

5bb4624

aayushijain23 reviewed Jun 18, 2019

View reviewed changes

colin-axner added 3 commits June 19, 2019 16:50

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into co…

c7fdc1f

…lin/4443-uniswap

reflect pr comments, fix some build issues in keeper/

9e84bf8

fixed some more build errors

cacbff3

golangcibot reviewed Jun 21, 2019

View reviewed changes

colin-axner commented Jun 21, 2019

View reviewed changes

colin-axner commented Jun 22, 2019

View reviewed changes

fedekunze added the WIP label Jun 28, 2019

colin-axner closed this Jun 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Uniswap Module #4518

WIP: Uniswap Module #4518

colin-axner commented Jun 7, 2019

fedekunze left a comment

AdityaSripal left a comment

AdityaSripal Jun 14, 2019

colin-axner Jun 14, 2019

AdityaSripal Jun 14, 2019

colin-axner Jun 14, 2019

fedekunze Jun 15, 2019

AdityaSripal Jun 14, 2019

AdityaSripal Jun 14, 2019

colin-axner Jun 14, 2019

aayushijain23 Jun 14, 2019

aayushijain23 Jun 17, 2019 •

edited

Loading

aayushijain23 Jun 18, 2019

aayushijain23 Jun 18, 2019

aayushijain23 Jun 18, 2019

golangcibot Jun 21, 2019

colin-axner Jun 21, 2019

AdityaSripal Jun 24, 2019

colin-axner Jun 22, 2019

fedekunze commented Jun 25, 2019

colin-axner commented Jun 28, 2019

WIP: Uniswap Module #4518

WIP: Uniswap Module #4518

Conversation

colin-axner commented Jun 7, 2019

fedekunze left a comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aayushijain23 Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze commented Jun 25, 2019

colin-axner commented Jun 28, 2019

aayushijain23 Jun 17, 2019 •

edited

Loading