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

chore: osmoutils, twap, downtime-detector, ibc-hooks cherry-pick to v14 #3925

Merged
merged 30 commits into from
Jan 5, 2023

Conversation

czarcas7ic
Copy link
Member

@czarcas7ic czarcas7ic commented Jan 4, 2023

Closes: #XXX

What is the purpose of the change

After internal discussions, we decided on launching v14 sooner with less features. This PR cherry picks the following features from main:

  • Geometric TWAP
  • Osmoutils refactor
  • Downtime Detector
  • IBC Hooks

Then, about four PRs were done manually by myself to fix issues that occurred due to the cherry picks.

nicolaslara and others added 20 commits January 4, 2023 13:59
* initial changes to migrate to ibc v4

* added checksum to proposal

* begin and end block are now being called inside nextBlock

* added changelog

* linked pr on changelog

* remove local replace

* using error acks from osmoutils

* osmoutils tagged

* go sum

* added checksum
* feat(x/twap): geometric twap code gen query boilerplate

* revert cli change

* query gen

* wire up API

* test

* fix

* fixes

* cli

* lint

* refactor via flag

* refactor

* refactor

* fixes

* lint

* add arithmetic twap alias
* moved ibc-hooks to its own go.mod

* updated ibc hooks version

* go sum

* add ics23 patch into x/ibc-hooks

* Fix wasm import version conflict

* Update x/ibc-hooks to osmoutils v0.0.2

* Update versions

Co-authored-by: Dev Ojha <[email protected]>
…etric twap (#3845)

* refactor(x/twap): handle spot price error case

* supporting test cases

* table-driven log tests
…3844)

* test(x/twap): add randomized test with a balancer pool

* comments

* multiplicative tolerance, fewer retries and larger initial supply range
* feat(x/twap): geometric twap code gen query boilerplate

* revert cli change

* query gen

* wire up API

* test

* fix

* fixes

* add geometric queries

* create pool.json

* add test

* resolve conflict

* fix: swap uosmo in

* fix problem with wallet creation

* updates

* simplify and add comments

* Update tests/e2e/e2e_test.go

* Update tests/e2e/e2e_test.go

* Update tests/e2e/configurer/chain/queries.go

Co-authored-by: Adam Tucker <[email protected]>

* Update tests/e2e/configurer/chain/queries.go

Co-authored-by: Adam Tucker <[email protected]>

* Update tests/e2e/e2e_test.go

Co-authored-by: Adam Tucker <[email protected]>

* Update tests/e2e/e2e_test.go

Co-authored-by: Adam Tucker <[email protected]>

* Update tests/e2e/e2e_test.go

Co-authored-by: Adam Tucker <[email protected]>

* Update tests/e2e/e2e_test.go

Co-authored-by: Adam Tucker <[email protected]>

Co-authored-by: Roman <[email protected]>
Co-authored-by: Roman <[email protected]>
Co-authored-by: Adam Tucker <[email protected]>
* feat(x/twap): GeometricTwap and GeometricTwapToNow queries added to Stargate whitelist

* update docs
* fix(scripts): proto gen for osmoutils

* Update scripts/protocgen.sh
* making osmoutils compile on chains that don't use osmosis' fork of the cosmos sdk

* updated imports for e2e tests

* go fumpt

* updated version everywhere

* added changelog entry
Sub-component of #3603

Adds types for the thin module intended for downtime detection

- Add downtime detection module types

No tests added

  - Does this pull request introduce a new feature or user-facing behavior changes? somewhat
  - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? yes
  - How is the feature or change documented? In its spec
* WIP

* Switch to enum

* Remove params query

* Add query

* Add wiring, add import/export test

* Start begin block test

* Finish keeper tests

* Add CLI

* Wire downtime detector CLI + queries

* more module wiring

* add types test

* Fix state alteration test

* Fix superfluid test

* Add stargate whitelist support

* Apply code comment

* Rename folder

* Add requested godoc

* Update x/downtime-detector/genesis.go

Co-authored-by: Adam Tucker <[email protected]>

* Apply adam suggestion for having a `-`

* move genesis test to own file

Co-authored-by: Adam Tucker <[email protected]>
* feat(osmomath): exp2 function

* export exp2

* changelog

* refactor ErrTolerance to use Dec instead of Int for additive tolerance

* Update osmomath/exp2.go

* Update osmomath/exp2.go

* Update osmomath/exp2.go

* Update osmomath/exp2_test.go

* Update osmomath/exp2_test.go

* do bit shift instead of multiplication

* godoc about error bounds

* comment about bit shift equivalency

* merge conflict

* improve godoc

* typo

* remove TODOs - confirmed obsolete

* Runge's phenomenon comment
* refactored twap api.go for geometric TWAP

* added barebon docs

* romans feedback

* new

* fix

* nichola feedback

* final roman comments
@github-actions github-actions bot added C:app-wiring Changes to the app folder C:CLI C:docs Improvements or additions to documentation C:x/gamm Changes, features and bugs related to the gamm module. C:x/superfluid C:x/twap Changes to the twap module T:build labels Jan 4, 2023
@czarcas7ic czarcas7ic changed the title chore: osmoutils, twap, downtime-detector, ibc-hooks cherry-pick chore: osmoutils, twap, downtime-detector, ibc-hooks cherry-pick to v14 Jan 4, 2023
@czarcas7ic czarcas7ic marked this pull request as ready for review January 4, 2023 21:59
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.

Confirmed that the following are the same as main:

  • x/twap
  • x/downtime-detector
  • x/ibc-hooks

Saw some diff in:

  • osmoutils
  • ibc-rate-limit

Noticed some PRs missing in:

Stargate whitelist LGTM

app/keepers LGTM

Comment on lines +93 to +100
// need to adapt s.val.String() to have an intermediate account,
// else the response is nil and theres a panic internally.
// {
// "Query estimate sfs delegated amount by validator & denom",
// "/osmosis.superfluid.Query/EstimateSuperfluidDelegatedAmountByValidatorDenom",
// &types.EstimateSuperfluidDelegatedAmountByValidatorDenomRequest{ValidatorAddress: s.val.String(), Denom: "gamm/pool/1"},
// &types.EstimateSuperfluidDelegatedAmountByValidatorDenomResponse{},
// },
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the problem here. Are we okay with commenting this out?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is commented out on main which is where it is coming from. Dev added this in his "Add downtime detector module" PR #3688

transfertypes "github.com/cosmos/ibc-go/v3/modules/apps/transfer/types"
clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types"
ibctesting "github.com/cosmos/ibc-go/v3/testing"
transfertypes "github.com/cosmos/ibc-go/v4/modules/apps/transfer/types"
Copy link
Member

Choose a reason for hiding this comment

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

I diffed x/ibc-rate-limit against main and there are some differences in this directory that I do not understand

Copy link
Member Author

Choose a reason for hiding this comment

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

The diffs are as follows (writing this for my own notes and for others to track/verify):

Copy link
Member Author

Choose a reason for hiding this comment

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

The above diffs were remedied with this commit
da63fa8

Copy link
Member

Choose a reason for hiding this comment

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

Confirmed that is the case ^

osmomath/exp2.go Outdated
for i := 1; i < len(numeratorCoefficients13Param); i++ {
x_exp_i.MulMut(x)

h_x = h_x.Add(numeratorCoefficients13Param[i].Mul(x_exp_i))
Copy link
Member

@p0mvn p0mvn Jan 4, 2023

Choose a reason for hiding this comment

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

This is missing an additional PR with updates about AddMut and MulMut. Please double-check that all osmomath PRs have been backported as I'm seeing some diff in binary search logic as well

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually did not think we were backporting all the osmomath PRs and only backported the ones that were needed from the twap logic. If this is a requirement I can go back and cherry pick them and/or just take the whole folder over

Copy link
Member Author

@czarcas7ic czarcas7ic Jan 5, 2023

Choose a reason for hiding this comment

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

If we did want to include everything from main from osmomath, the following is what is missing:

Copy link
Member

Choose a reason for hiding this comment

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

We should have: #3779

The rest makes sense to not have to me

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! f864dcf

Copy link
Member

Choose a reason for hiding this comment

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

Going to add #3839 in the Ibc-hooks PR

@@ -3,11 +3,25 @@ package osmoutils
import (
"encoding/json"

Copy link
Member

Choose a reason for hiding this comment

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

Seeing some diff in osmoutils from main. Some of the are accum package which makes sense but there are also a few other helper changes.

They seem unrelated and acceptable to me but commenting for visibility

Copy link
Member Author

Choose a reason for hiding this comment

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

The diff is as follows:

Fairly confident all these are not needed

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me to not have all these, thanks for summarizing

@github-actions github-actions bot added the T:CI label Jan 5, 2023
* mut add

* test add mut

* quo  mut

* test quo mut/ remove want from test struct

* refactor exp

* change mutatives code

* change

* not allocaing

* exp change to quomut

* remove file

* refactor quo

* refactor ad

* refactor tests
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.

LGTM. Thank you @czarcas7ic

My review is based on diffing the relevant modules, osmomath, osmotutils, and app folders against main.

Only skimmed other changes.

Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

Reviewed by comparing individual commits from main LGTM

Wondering if we can get

Also wondering if it is possible to update CHANGELOG to latest changes (v14 section & unreleased section) I'm concerned about the case that we might lose track of commits that
we've made to main and then they would be forgotten at some point(If you agree, I could also help with organizing changelog)

mattverse and others added 4 commits January 5, 2023 15:20
* whitelist EstimateSwapExactAmountOut

* doc: changelog
* added cargo.lock

* added Cargo.lock as an artifact

* added new bytecode with lock file
@nicolaslara
Copy link
Contributor

It makes sense that the contract tests would fail if the bytecode was fetched from main. The Cargo.lock file didn't exist in this branch since #3576 was not inclided. I've cherry picked it.

@czarcas7ic czarcas7ic merged commit 961738a into v14.x Jan 5, 2023
@czarcas7ic czarcas7ic deleted the adam/cherry-pick-from-main branch January 5, 2023 16:40
)
appKeepers.IBCHooksKeeper = &hooksKeeper

appKeepers.WireICS20PreWasmKeeper(appCodec, bApp, appKeepers.IBCHooksKeeper)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for adding these manually instead of cherry picking #3589 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It might have just been my confusion dealing with multiple cherry picks

node.StoreWasmCode("rate_limiter.wasm", initialization.ValidatorWalletName)
chainA.LatestCodeId += 1
chainA.LatestCodeId = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

@czarcas7ic This looks good, but similar question. Why not cherry pick #3635?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think my main concern was I didn't want to cherry-pick a PR and not realize it had some drive by that it shouldn't have

@ValarDragon
Copy link
Member

PR LGTM, great job on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:app-wiring Changes to the app folder C:CLI C:docs Improvements or additions to documentation C:x/gamm Changes, features and bugs related to the gamm module. C:x/superfluid C:x/twap Changes to the twap module T:build T:CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants