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

reward converter - add trade route ID to portId #1011

Merged
merged 7 commits into from
Dec 7, 2023

Conversation

sampocs
Copy link
Collaborator

@sampocs sampocs commented Dec 5, 2023

Context

We need to have a separate trade ICA for each trade route. Otherwise, if two trade routes shared a common denom, it could interfere with the queries.

Brief Changelog

  • Added FormatTradeRouteICAOwner and FormatTradeRouteICAOwnerFromAccount
  • Renamed FormatICAAccountOwner to FormatHostZoneICAOwner
  • Updated trade route ICA owners to use the new function
  • Added unit tests for the relevant types functions

NOTE: @ethan-stride, I'm not really loving the implementation of these owner functions. Curious if you can think of a better way to do it

Copy link
Contributor

@ethan-stride ethan-stride left a comment

Choose a reason for hiding this comment

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

LGTM
Lots of places I would think, surely we can combine these two functions, or wow that seems wordy or there has to be a way to make the function take fewer inputs.... and each time when I would look at the minimum information needed to accomplish the goal you would be right. It is well done but it doesn't feel elegant because this particular piece of code can't be elegant no matter how we try to wrap and contain the interfaces between functions. Managing the async life cycle for ICAs which don't exist on a hostZone struct are just going to carry around a lot of extra fields no matter what, I think this is about the best we can do for the inherent complexity of this part of the system

app/apptesting/test_helpers.go Show resolved Hide resolved
x/stakeibc/keeper/msg_server_create_trade_route.go Outdated Show resolved Hide resolved
x/stakeibc/types/ica_account.go Show resolved Hide resolved
x/stakeibc/types/keys.go Show resolved Hide resolved
@sampocs
Copy link
Collaborator Author

sampocs commented Dec 5, 2023

@ethan-stride what are you thoughts on the FormatTradeRouteICAOwnerFromAccount? Does it make sense to pass the full account or instead do

func FormatTradeRouteICAOwnerFromAccount(chainId, tradeRouteId string, icaAccountType ICAAccountType) string {
	return fmt.Sprintf("%s.%s.%s", chainId, tradeRouteId, icaAccountType.String())
}

it would muddy the invocation a bit

// from 
owner := types.FormatTradeRouteICAOwnerFromAccount(route.GetRouteId(), route.TradeAccount)
// to
tradeAccount := route.TradeAccount
owner := types.FormatTradeRouteICAOwnerFromAccount(tradeAccount.ChainId, route.GetRouteId(), tradeAccount.Type)

But it would be a little more explicit about which fields are used instead of having to know that the ICAAccount struct needs both chainId and type filled out

@ethan-stride
Copy link
Contributor

@ethan-stride what are you thoughts on the FormatTradeRouteICAOwnerFromAccount? Does it make sense to pass the full account or instead do

func FormatTradeRouteICAOwnerFromAccount(chainId, tradeRouteId string, icaAccountType ICAAccountType) string {
	return fmt.Sprintf("%s.%s.%s", chainId, tradeRouteId, icaAccountType.String())
}

it would muddy the invocation a bit

// from 
owner := types.FormatTradeRouteICAOwnerFromAccount(route.GetRouteId(), route.TradeAccount)
// to
tradeAccount := route.TradeAccount
owner := types.FormatTradeRouteICAOwnerFromAccount(tradeAccount.ChainId, route.GetRouteId(), tradeAccount.Type)

But it would be a little more explicit about which fields are used instead of having to know that the ICAAccount struct needs both chainId and type filled out

From discussion on Slack: being explicit about which fields from the account are going into the function call makes it harder to mistakenly pass in an account which is not yet initialized without realizing it. This makes the function clearer but calling it slightly more work -- overall a tradeoff which is probably worth it so the new proposed function params make sense.

@ethan-stride
Copy link
Contributor

LGTM I think it is ready

@sampocs sampocs merged commit 981bfa3 into autoswap-revenue-tokens Dec 7, 2023
1 check passed
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.

2 participants