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: cw dex bindings #365

Merged
merged 20 commits into from
Feb 26, 2024
Merged

Feat: cw dex bindings #365

merged 20 commits into from
Feb 26, 2024

Conversation

pr0n00gler
Copy link
Collaborator

@pr0n00gler pr0n00gler commented Nov 14, 2023

The aim of the PR is to implement binding cosmwasm interface for dex module messages and queries.
I've implemented binding adapters https://github.com/neutron-org/neutron/blob/feat/cw-dex-bindings/x/dex/types/wasm.go. The purpose of the adapters to update original dex types: expiration_time changed from Timetamp to u64 unixtime, some field names converted to snake_case.
related PRs:
neutron-org/neutron-sdk#120
neutron-org/neutron-dev-contracts#34
neutron-org/neutron-integration-tests#240
neutron-org/neutronjsplus#24

type Dex struct {
Deposit *types.MsgDeposit `json:"deposit"`
Withdrawal *types.MsgWithdrawal `json:"withdrawal"`
PlaceLimitOrder *MsgPlaceLimitOrder `json:"place_limit_order"`
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't reuse *types.MsgPlaceLimitOrder? bc of the ExpirationTime format?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like the copy+paste from proto file with subtle changes made: on proto updates, we'll have to manually reflect the changes in such copy+pasted structures. what do you think about making kind of wrapper structs for each struct where we replace time.Time with uint64 and implementing a simple custom UnmarshalJSON method for them. rough example (compiles but not sure that works):

type QueryEstimatePlaceLimitOrderRequest dextypes.QueryEstimatePlaceLimitOrderRequest

func (r *QueryEstimatePlaceLimitOrderRequest) MarshalJSON() ([]byte, error) {
	type Alias dextypes.QueryEstimatePlaceLimitOrderRequest
	return json.Marshal(&struct {
		ExpirationTime int64 `json:"expiration_time"`
		*Alias
	}{
		ExpirationTime: r.ExpirationTime.Unix(),
		Alias:          (*Alias)(r),
	})
}

func (r *QueryEstimatePlaceLimitOrderRequest) UnmarshalJSON(data []byte) error {
	type Alias dextypes.QueryEstimatePlaceLimitOrderRequest
	aux := &struct {
		ExpirationTime int64 `json:"expiration_time"`
		*Alias
	}{
		Alias: (*Alias)(r),
	}
	if err := json.Unmarshal(data, &aux); err != nil {
		return err
	}
	expirationTime := time.Unix(aux.ExpirationTime, 0)
	r.ExpirationTime = &expirationTime
	return nil
}

The point is if we ever change the proto e.g. add/remove fields, change some fields type, etc, we won't have to reflect the changes here in the copy+pasted structure. it will work under the hood because of the Alias auxiliary type.

custom unmarshal JSON guide: https://choly.ca/post/go-json-marshalling/

network/init-neutrond.sh Outdated Show resolved Hide resolved
wasmbinding/queries.go Outdated Show resolved Hide resolved
wasmbinding/bindings/query.go Outdated Show resolved Hide resolved
wasmbinding/bindings/query.go Outdated Show resolved Hide resolved
wasmbinding/queries.go Show resolved Hide resolved
wasmbinding/message_plugin.go Outdated Show resolved Hide resolved
return nil, errors.Wrap(err, "marshal json failed")
}

ctx.Logger().Debug(fmt.Sprintf("%T execution completed", handler),
Copy link
Contributor

Choose a reason for hiding this comment

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

"%T execution completed", handler

must be msg instead, right?

"msg", resp,
"error", err,
)
return nil, errors.Wrap(err, "marshal json failed")
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make a Wrapf here with msg's %T printed as well

}

func (m *CustomMessenger) dispatchDexMsg(ctx sdk.Context, contractAddr sdk.AccAddress, dex bindings.Dex) ([][]byte, error) {
if dex.Deposit != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice and clean to use a switch statement here for msg cases just as you did for queries

@sotnikov-s
Copy link
Contributor

guys please update the PR's description with some explanation and links to related PRs

@pr0n00gler pr0n00gler marked this pull request as draft November 17, 2023 14:22
TokenOut string `json:"token_out,omitempty"`
TickIndexInToOut int64 `json:"tick_index_in_to_out,omitempty"`
AmountIn math.Int `json:"amount_in"`
OrderType types.LimitOrderType `json:"order_type,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

let's consider having the OrderType type as string and then handled the same way as here:

orderTypeInt, ok := types.LimitOrderType_value[args[5]]

reasoning is raw types.LimitOrderType at user's side is represented as numbers (0, 1, 2, 3 or 4, proof). would be better to expect concrete string values and compare them to the enumerated string values

@swelf19 swelf19 force-pushed the feat/cw-dex-bindings branch from 4567cdb to 188613c Compare January 26, 2024 09:11
@swelf19 swelf19 changed the base branch from chore/duality-proto-camelCase->snake_case to main January 26, 2024 09:11
@pr0n00gler pr0n00gler marked this pull request as ready for review February 16, 2024 10:05
testutil/test_helpers.go Outdated Show resolved Hide resolved
wasmbinding/bindings/msg.go Outdated Show resolved Hide resolved
wasmbinding/bindings/query.go Outdated Show resolved Hide resolved
quasisamurai
quasisamurai previously approved these changes Feb 19, 2024
wasmbinding/bindings/msg.go Show resolved Hide resolved
wasmbinding/bindings/query.go Show resolved Hide resolved
wasmbinding/custom_querier.go Outdated Show resolved Hide resolved
wasmbinding/message_plugin.go Show resolved Hide resolved
wasmbinding/message_plugin.go Outdated Show resolved Hide resolved
@pr0n00gler
Copy link
Collaborator Author

pr0n00gler commented Feb 21, 2024

I can't approve the PR since explicitly, since i opened it, but i approve it

@pr0n00gler pr0n00gler merged commit 99f8548 into main Feb 26, 2024
7 checks passed
@pr0n00gler pr0n00gler deleted the feat/cw-dex-bindings branch August 7, 2024 08:31
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.

5 participants