-
Notifications
You must be signed in to change notification settings - Fork 607
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
fix(ProtoRev): Parameterizing Pool Type Information #5948
Conversation
// CosmwasmPoolInfo contains meta data pertaining to a cosmwasm pool type. | ||
message CosmwasmPoolInfo { | ||
// The weight of a cosmwasm pool (by contract address) | ||
map<string, uint64> weight_map = 1 [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if using a map is problematic for app hashing. We do no iterations on the map itself in the module. The primary use case is determining the number of pool weight of a given cosmwasm pool (which may vary across applications which is why its mapped to contract addresses).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ValarDragon can you verify the safety of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had instances where we tried to use maps quite a while ago, I think the decision back then was to try avoid using maps for extra safety, would it be possible to change it so tht we use repeated structs instead here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it. ty for confirming
x/protorev/client/cli/tx.go
Outdated
}, | ||
"cosmwasm" : { | ||
"weight_map" : { | ||
"ibc/123..." : 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"ibc/123..." : 1, | |
"osmo123..." : 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: cosmwasm weight map represents address to weight, so use an address placeholder instead of an asset placeholder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, left comments on the benchmark decisions
Base weight on CL pools should be around 7 (takes 7ms to execute a route with a CL pool). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything except for the map in the proto and the changed proto field looks good to me. Will need a second ack before merge. Thank you!
// Information that is used to estimate execution time / gas | ||
// consumption of a swap on a given pool type. | ||
InfoByPoolType info_by_pool_type = 4 [ | ||
(gogoproto.nullable) = false, | ||
(gogoproto.moretags) = "yaml:\"pool_weights\"" | ||
(gogoproto.moretags) = "yaml:\"info_by_pool_type\"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are renaming the proto field, we should depreciate this field and add a new one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Or reserve!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, add a note that this is deprecated and adding a new field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added back the previous field with a note that it is being deprecated.
// CosmwasmPoolInfo contains meta data pertaining to a cosmwasm pool type. | ||
message CosmwasmPoolInfo { | ||
// The weight of a cosmwasm pool (by contract address) | ||
map<string, uint64> weight_map = 1 [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ValarDragon can you verify the safety of this?
Use: "info-by-pool-type", | ||
Short: "Query the pool info used to determine how computationally expensive a route is", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume there is an input right? I think this should be "info-by-pool-type [pool-type]"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea the input is the json file with all of the correct pool info. This isnt used to update by a single pool type (although once the dependencies on a given pool type grow it might be worth migrating to separate message handlers).
I'll update the PR with the json example I used in testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, except for using repeated structs as an alternative for maps
@czarcas7ic @mattverse PR is updated with the removal of the map struct! Please lmk if there are any other blocks on your end (since I know the code freeze should be happening soon). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not review details of calculation going on for CL pools, otherwise LGTM
* init * testing update * changelog * comments * nit * refactor * clean up * nit * comment * cr * cl are not symetric * init * smh more symetric changes * simpler safe swap * map update * more stuff * testing fixes * more testing * nits * benchmark + CLI clean up * test fix * no map no problems * update with merge * better docs
What is the purpose of the change
In this PR, I refactor the way we track information about different pool types in ProtoRev. In particular, currently each pool type has an associated pool weight (estimated amount of time in ms it takes to make a swap on that pool). However, with the addition of Cosmwasm and CL pools, we will likely need to make more assumptions based on pool type. The most basic one is
MaxTicksCrossed
which denotes the number of ticks we are willing to cross in our binary search.This update will allow us to have more control over how ProtoRev responds to different pool types.
Testing and Verifying
Documentation and Release Note
Unreleased
section ofCHANGELOG.md
?This is related to the over-arching issue tracking protorev V17 updates. #5858