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

ProtoRev: Adding Txs to CLI #4567

Merged
merged 4 commits into from
Mar 24, 2023
Merged

ProtoRev: Adding Txs to CLI #4567

merged 4 commits into from
Mar 24, 2023

Conversation

davidterpay
Copy link
Contributor

@davidterpay davidterpay commented Mar 10, 2023

What is the purpose of the change

In this PR, I add the remaining admin transactions to the CLI. Additionally, I add one modification to MsgSetMaxPoolPointsPer(Tx/Block) so that the max pool points per tx cannot be set to something higher than the max pool points per block and vice versa.

Brief Changelog

  • Added all protorev txs to the cli
  • Added an additional check to SetMaxPoolPointsPerTx and SetMaxPoolPointsPerBlock
  • Update the protorev spec

Testing and Verifying

This change is already covered by existing tests. Additionally, I tested all of the CLI commands locally with accounts that were designated as the admin account and those that were not, with various params (correct and incorrect), and more. The CLI commands function as expected.

Documentation and Release Note

Updated the x/protorev spec to include all of the updates made in this PR.

@github-actions github-actions bot added C:CLI C:docs Improvements or additions to documentation labels Mar 10, 2023
@davidterpay davidterpay added V:state/breaking State machine breaking PR and removed C:docs Improvements or additions to documentation C:CLI labels Mar 10, 2023
@github-actions github-actions bot added C:CLI C:docs Improvements or additions to documentation labels Mar 10, 2023
@davidterpay davidterpay changed the title Terpay/protorev cli update ProtoRev: Adding Txs to CLI Mar 10, 2023
@davidterpay davidterpay marked this pull request as ready for review March 13, 2023 21:36
@davidterpay davidterpay requested a review from p0mvn March 13, 2023 21:36
Comment on lines +63 to +95
func (release *createArbRoutesInput) extractTokenPairArbRoutes() []types.TokenPairArbRoutes {
if release == nil {
return nil
}

tokenPairArbRoutes := make([]types.TokenPairArbRoutes, 0)

// Iterate through each hot route and construct the token pair arb routes
for _, hotRoute := range *release {
current := types.TokenPairArbRoutes{}
current.TokenIn = hotRoute.TokenIn
current.TokenOut = hotRoute.TokenOut

for _, arbRoute := range hotRoute.ArbRoutes {
currentArbRoute := types.Route{}
currentArbRoute.StepSize = sdk.NewIntFromUint64(arbRoute.StepSize)

for _, trade := range arbRoute.Trades {
currentTrade := types.Trade{}
currentTrade.Pool = trade.Pool
currentTrade.TokenIn = trade.TokenIn
currentTrade.TokenOut = trade.TokenOut
currentArbRoute.Trades = append(currentArbRoute.Trades, currentTrade)
}

current.ArbRoutes = append(current.ArbRoutes, currentArbRoute)
}

tokenPairArbRoutes = append(tokenPairArbRoutes, current)
}

return tokenPairArbRoutes
}
Copy link
Contributor Author

@davidterpay davidterpay Mar 13, 2023

Choose a reason for hiding this comment

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

To reviewers: I'm not super familiar with osmocli so I'm not sure if there is a more optimal way of reading in the json file and building the TokenPairArbRoutes since the only thing that is problematic is dealing with the StepSize which is an sdk.Int.

Copy link
Member

Choose a reason for hiding this comment

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

The only other example I know of parsing a file withing our cli is the pool file:

// UnmarshalJSON should error if there are fields unexpected.
func (release *createBalancerPoolInputs) UnmarshalJSON(data []byte) error {
var createPoolE XCreatePoolInputsExceptions
dec := json.NewDecoder(bytes.NewReader(data))
dec.DisallowUnknownFields() // Force
if err := dec.Decode(&createPoolE); err != nil {
return err
}
*release = createBalancerPoolInputs(createPoolE.XCreatePoolInputs)
return nil
}

This implementation seems to be aligned with that. If parsing files becomes a common theme, we can better abstract it in the future

Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

utACK

Thanks for this change and for testing it out locally

@@ -71,6 +71,15 @@ func (m MsgServer) SetMaxPoolPointsPerTx(c context.Context, msg *types.MsgSetMax
return nil, err
}

maxPointsPerBlock, err := m.k.GetMaxPointsPerBlock(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking but just FYI for future contributions that it is beneficial to separate state-breaking changes from non-state-breaking so that we can release non-state-breaks sooner.

We have docs for this here:

### State-compatibility

If we only had CLI changes in this PR, we could release them in the v15 line. Since there are message server changes, this will have to wait till v16.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Will keep that in mind once we start working on the metrics/events update.

Comment on lines +63 to +95
func (release *createArbRoutesInput) extractTokenPairArbRoutes() []types.TokenPairArbRoutes {
if release == nil {
return nil
}

tokenPairArbRoutes := make([]types.TokenPairArbRoutes, 0)

// Iterate through each hot route and construct the token pair arb routes
for _, hotRoute := range *release {
current := types.TokenPairArbRoutes{}
current.TokenIn = hotRoute.TokenIn
current.TokenOut = hotRoute.TokenOut

for _, arbRoute := range hotRoute.ArbRoutes {
currentArbRoute := types.Route{}
currentArbRoute.StepSize = sdk.NewIntFromUint64(arbRoute.StepSize)

for _, trade := range arbRoute.Trades {
currentTrade := types.Trade{}
currentTrade.Pool = trade.Pool
currentTrade.TokenIn = trade.TokenIn
currentTrade.TokenOut = trade.TokenOut
currentArbRoute.Trades = append(currentArbRoute.Trades, currentTrade)
}

current.ArbRoutes = append(current.ArbRoutes, currentArbRoute)
}

tokenPairArbRoutes = append(tokenPairArbRoutes, current)
}

return tokenPairArbRoutes
}
Copy link
Member

Choose a reason for hiding this comment

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

The only other example I know of parsing a file withing our cli is the pool file:

// UnmarshalJSON should error if there are fields unexpected.
func (release *createBalancerPoolInputs) UnmarshalJSON(data []byte) error {
var createPoolE XCreatePoolInputsExceptions
dec := json.NewDecoder(bytes.NewReader(data))
dec.DisallowUnknownFields() // Force
if err := dec.Decode(&createPoolE); err != nil {
return err
}
*release = createBalancerPoolInputs(createPoolE.XCreatePoolInputs)
return nil
}

This implementation seems to be aligned with that. If parsing files becomes a common theme, we can better abstract it in the future

@p0mvn p0mvn merged commit 243b594 into main Mar 24, 2023
@p0mvn p0mvn deleted the terpay/protorev-cli-update branch March 24, 2023 15:46
czarcas7ic added a commit that referenced this pull request Mar 25, 2023
…ug (#4735)

* link (#4708)

* daily check for broken links (#4712)

* Update README.md

* Create check-broken-links.yml

* Update check-broken-links.yml

* Update check-broken-links.yml

* Update check-broken-links.yml

* Update check-broken-links.yml

* Update check-broken-links.yml

* Update check-broken-links.yml

* Update README.md

* Update AutoStake seed from .net to .com (#4711)

Co-authored-by: AutoStake <nathan@blockngine.io>

* [tests/e2e]: simplify the logic for adding accounts to genesis sdk side and integrate into e2e (#4706)

* update logic add account genesis

* clean unused package

* golint

* XCS + Registries integration: IBC forward generation using registries (#4694)

* initial attempt at splitting registries. Issues with circular deps

* properly split registries

* added reverse prefix map

* channel validation done by registries

* integrating registry into xcs and fixing the tests

* Printing error context on failure

* full integration with unwrapping

* remove replace

* removed unused deps

* clippy

* x86 bytecode

* fmt after lints

* gofumpt

* fix re-export

* lint

* added test-contract addr

* added missing newline

* remove recovery states

* x86 bytecode

* new bytecode

* added single pool query (#4549)

* added single pool query

* added changelog

* remove empty tests as they are invalid

* removed sender and made reserved

* remove and reserve unused sender

* removed sender

* actually remove sender

* querygen

* move impl to correct file

* more flexible querygen

* Update proto/osmosis/poolmanager/v1beta1/query.proto

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

* Update proto/osmosis/poolmanager/v1beta1/query.proto

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

* Update proto/osmosis/poolmanager/v1beta1/query.proto

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

* Update proto/osmosis/poolmanager/v1beta1/query.proto

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

* protos

* added cli

* fixed command parsing

* single line

* lint

* fix cli

* run proto gen

* added whitelisted query

* fix test for v15

---------

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

* Revert "[tests/e2e]: simplify the logic for adding accounts to genesis sdk side and integrate into e2e (#4706)" (#4727)

This reverts commit a9725d6.

* ProtoRev: Adding Txs to CLI (#4567)

* init set hot routes cli command

* adding remaining txs

* few more tests

* comments

* Update upgrade_test.go (#4734)

* refactor tests and fix incentive forfeiting bug

* update create position tx

* feat: whitelist poolmanager Pool, CL Params and ClaimableFees queries (#4725)

* feat: whitelist poolmanager Pool, CL Params and ClaimableFees queries

* lint

* conflict

* fix e2e query parsing

* increase gas

---------

Co-authored-by: Ruslan Akhtariev <46343690+pysel@users.noreply.github.com>
Co-authored-by: Master Pi <minh.vln140501@gmail.com>
Co-authored-by: AutoStake <22457974+AutoStake-com@users.noreply.github.com>
Co-authored-by: AutoStake <nathan@blockngine.io>
Co-authored-by: Nguyen Thanh Nhan <thanhnhan98qh@gmail.com>
Co-authored-by: Nicolas Lara <nicolaslara@gmail.com>
Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>
Co-authored-by: David Terpay <35130517+davidterpay@users.noreply.github.com>
Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
Co-authored-by: Roman <roman@osmosis.team>
czarcas7ic added a commit that referenced this pull request Mar 30, 2023
* utilize new position id in position key

* add position id to migration response

* regen proto

* modify withdraw pos in e2e

* cmd order

* avoid store key formatting with fmt.Sprintf

* set default NextPositionId to 1

* lint

* return positionId from CreatePos E2E

* utilize bytes.Buffer to build keys

* Revert "utilize bytes.Buffer to build keys"

This reverts commit 0c1bc13.

* utilize sprintf for key

* remove extra positions logic

* initial push

* regen proto

* Update go.mod

* lints

* fix sim

* add posid to sim collectincentive

* e2e test fix

* lint

* clean up

* remove extra fields

* regen proto

* only use pos id for fees

* update cli and e2e

* fee accum updates

* modify claim to use only positionId

* fix functional fees test

* pool to position id map

* add pool store to delete position

* fix conflicts from main merge

* test updates

* lints / cleanup

* add position Id to create position response

* regen proto

* regen proto

* switch order

* regen proto

* add error comment to collectFees

* move key format to types

* rename to positionFields

* Remove unnecessary test field

* revert refactor to functional fees test

* define custom errors

* remove duplicate events emitted

* correct validatePositionUpdate inputs

* Update x/concentrated-liquidity/position.go

Co-authored-by: Roman <roman@osmosis.team>

* update README for KV stores

* other readme updates

* feat: query position by id (added to stargate whitelist) (#4718)

* feat(CL): position by id query (stargate whitelisted)

* fix test

* Update app/upgrades/v15/upgrade_test.go

---------

Co-authored-by: Adam Tucker <adam@osmosis.team>

* initial push

* remove position age as input param

* [CL][bugfix] Refactor incentives tests and fix incentive forfeiting bug (#4735)

* link (#4708)

* daily check for broken links (#4712)

* Update README.md

* Create check-broken-links.yml

* Update check-broken-links.yml

* Update check-broken-links.yml

* Update check-broken-links.yml

* Update check-broken-links.yml

* Update check-broken-links.yml

* Update check-broken-links.yml

* Update README.md

* Update AutoStake seed from .net to .com (#4711)

Co-authored-by: AutoStake <nathan@blockngine.io>

* [tests/e2e]: simplify the logic for adding accounts to genesis sdk side and integrate into e2e (#4706)

* update logic add account genesis

* clean unused package

* golint

* XCS + Registries integration: IBC forward generation using registries (#4694)

* initial attempt at splitting registries. Issues with circular deps

* properly split registries

* added reverse prefix map

* channel validation done by registries

* integrating registry into xcs and fixing the tests

* Printing error context on failure

* full integration with unwrapping

* remove replace

* removed unused deps

* clippy

* x86 bytecode

* fmt after lints

* gofumpt

* fix re-export

* lint

* added test-contract addr

* added missing newline

* remove recovery states

* x86 bytecode

* new bytecode

* added single pool query (#4549)

* added single pool query

* added changelog

* remove empty tests as they are invalid

* removed sender and made reserved

* remove and reserve unused sender

* removed sender

* actually remove sender

* querygen

* move impl to correct file

* more flexible querygen

* Update proto/osmosis/poolmanager/v1beta1/query.proto

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

* Update proto/osmosis/poolmanager/v1beta1/query.proto

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

* Update proto/osmosis/poolmanager/v1beta1/query.proto

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

* Update proto/osmosis/poolmanager/v1beta1/query.proto

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

* protos

* added cli

* fixed command parsing

* single line

* lint

* fix cli

* run proto gen

* added whitelisted query

* fix test for v15

---------

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

* Revert "[tests/e2e]: simplify the logic for adding accounts to genesis sdk side and integrate into e2e (#4706)" (#4727)

This reverts commit a9725d6.

* ProtoRev: Adding Txs to CLI (#4567)

* init set hot routes cli command

* adding remaining txs

* few more tests

* comments

* Update upgrade_test.go (#4734)

* refactor tests and fix incentive forfeiting bug

* update create position tx

* feat: whitelist poolmanager Pool, CL Params and ClaimableFees queries (#4725)

* feat: whitelist poolmanager Pool, CL Params and ClaimableFees queries

* lint

* conflict

* fix e2e query parsing

* increase gas

---------

Co-authored-by: Ruslan Akhtariev <46343690+pysel@users.noreply.github.com>
Co-authored-by: Master Pi <minh.vln140501@gmail.com>
Co-authored-by: AutoStake <22457974+AutoStake-com@users.noreply.github.com>
Co-authored-by: AutoStake <nathan@blockngine.io>
Co-authored-by: Nguyen Thanh Nhan <thanhnhan98qh@gmail.com>
Co-authored-by: Nicolas Lara <nicolaslara@gmail.com>
Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>
Co-authored-by: David Terpay <35130517+davidterpay@users.noreply.github.com>
Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
Co-authored-by: Roman <roman@osmosis.team>

* refactor collect incentives test

* remove any final references to freezing

* merge main

* add back ParsePositionFromBz

* add comments to test

* add forfeit incentives withdraw test case

* add negative duration validation and test

* clean up

* add back multi-position test

* regen proto

---------

Co-authored-by: czarcas7ic <czarcas7ic@users.noreply.github.com>
Co-authored-by: Roman <roman@osmosis.team>
Co-authored-by: alpo <62043214+AlpinYukseloglu@users.noreply.github.com>
Co-authored-by: Ruslan Akhtariev <46343690+pysel@users.noreply.github.com>
Co-authored-by: Master Pi <minh.vln140501@gmail.com>
Co-authored-by: AutoStake <22457974+AutoStake-com@users.noreply.github.com>
Co-authored-by: AutoStake <nathan@blockngine.io>
Co-authored-by: Nguyen Thanh Nhan <thanhnhan98qh@gmail.com>
Co-authored-by: Nicolas Lara <nicolaslara@gmail.com>
Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>
Co-authored-by: David Terpay <35130517+davidterpay@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:CLI C:docs Improvements or additions to documentation V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants