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

refactor(deps): switch to cosmossdk.io/math from fork math #6238

Merged
merged 3 commits into from
Sep 1, 2023

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Aug 30, 2023

Progress towards: #6099

What is the purpose of the change

This PR proposes creating an alias for sdkmath types in osmomath.

Primarily, its goal is to remove the need for the verbose and unreadable prefix "Legacy" from Dec.

To avoid having to directly import both sdkmath and osmomath in files where both Dec and Int/Uint types are used we also create aliases for integer types. The aliases are created in the new file sdk_math_alias.go

To avoid name clashes with BigDec and BigDec, their constrictor APIs are now broken to contain "Big" in the suffix. For example, NewDec -> NewBigDec.

Important Notes

  • Note: with the update, the Dec and Int do not get initialized to zero values
    by default in proto marhaling/unmarshaling. Instead, they get set to nil values.
  • maxDecBitLen has changed by one bit so overflow panic can be triggerred sooner.
  • Had to update precompute.go in CL due to maxBitLen changes
  • For converting Int to Dec, "LegacyDec" is still present since it is impossible to define a method on Int outside of cosmossdk.io/math package
  • I suggest we do not backport this due to complexity of resolving all these issues and the changes to maxDecBitLen + proto marshaling

Testing and Verifying

This change is already covered by existing tests

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

@github-actions
Copy link
Contributor

github-actions bot commented Aug 30, 2023

Important Notice

This PR modifies an in-repo Go module. It is one of:

  • osmomath
  • osmoutils
  • x/ibc-hooks
  • x/epochs

The dependent Go modules, especially the root one, will have to be
updated to reflect the changes. Failing to do so might cause e2e to fail.

Please follow the instructions below:

  1. Open https://github.com/osmosis/osmosis/actions/workflows/go-mod-auto-bump.yml
  2. Provide the current branch name
  3. On success, confirm if an automated commit corretly updated the go.mod and go.sum files

Please let us know if you need any help.

@github-actions github-actions bot added C:docs Improvements or additions to documentation C:x/lockup C:CLI C:x/gamm Changes, features and bugs related to the gamm module. C:x/mint C:x/incentives C:x/epochs C:x/pool-incentives C:x/superfluid C:x/txfees C:app-wiring Changes to the app folder C:x/tokenfactory C:simulator Edits simulator or simulations C:x/twap Changes to the twap module C:x/concentrated-liquidity C:x/poolmanager labels Aug 30, 2023
@p0mvn p0mvn added the V:state/breaking State machine breaking PR label Aug 30, 2023
@p0mvn p0mvn marked this pull request as ready for review August 30, 2023 19:26
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.

Just curious, what's the reason for decision behind making an alias package osmomath and then importing from osmomath directly instead of using sdkmath package?

@p0mvn
Copy link
Member Author

p0mvn commented Aug 31, 2023

Just curious, what's the reason for decision behind making an alias package osmomath and then importing from osmomath directly instead of using sdkmath package?

This is to reduce verbosity from the LegacyDec API stemming from upstream

@p0mvn p0mvn requested a review from mattverse August 31, 2023 16:54
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.

LGTM, nice work, must have been pain going through all this

Comment on lines +78 to +80
func ZeroBigDec() BigDec { return BigDec{new(big.Int).Set(zeroInt)} }
func OneBigDec() BigDec { return BigDec{precisionInt()} }
func SmallestBigDec() BigDec { return BigDec{new(big.Int).Set(oneInt)} }
Copy link
Member

Choose a reason for hiding this comment

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

I like these changes, feels like this is what it should have always been

Copy link
Contributor

@PaddyMc PaddyMc left a comment

Choose a reason for hiding this comment

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

Awesome work with this PR, reviewed an it works as expected 👍

I have one comment based on the use of sdk.ZeroDec( and other uses of the sdk types package for math functions. There's an alias created there that is very similar to the alias created in the osmomath package.

When we run grep -r "sdk.ZeroDec(" we find we refer to the alias in our sdk fork, could we just use osmomath.ZeroDec( here instead? And remove the need to import the sdk "github.com/cosmos/cosmos-sdk/types" package for math functions?

Approved here with the understanding that this suggestion should probably be another PR if we want to go that route. ⚡

@p0mvn
Copy link
Member Author

p0mvn commented Sep 1, 2023

grep -r "sdk.ZeroDec("

Seems that some updates got affected by merge conflicts, thanks for catching. Will reswitch everything prior to merge

@p0mvn p0mvn added A:backport/v19.x backport patches to v19.x branch A:backport/v18.x backport patches to v18.x branch and removed A:backport/v19.x backport patches to v19.x branch labels Sep 1, 2023
@p0mvn p0mvn merged commit ca75f4c into main Sep 1, 2023
1 check passed
@p0mvn p0mvn deleted the roman/unfork-sdkmath branch September 1, 2023 15:18
mergify bot pushed a commit that referenced this pull request Sep 1, 2023
* refactor(deps): switch to cosmossdk.io/math from fork math

* switch remaining things

* updates

(cherry picked from commit ca75f4c)

# Conflicts:
#	app/apptesting/concentrated_liquidity.go
#	app/apptesting/pool_manager.go
#	app/apptesting/test_suite.go
#	app/upgrades/v16/upgrades.go
#	app/upgrades/v17/constants.go
#	app/upgrades/v18/upgrades_test.go
#	app/upgrades/v19/upgrades.go
#	app/upgrades/v19/upgrades_test.go
#	app/upgrades/v3/forks.go
#	app/upgrades/v9/prop214.go
#	cmd/osmosisd/cmd/genesis.go
#	cmd/osmosisd/cmd/root.go
#	go.mod
#	go.sum
#	osmomath/sqrt.go
#	osmomath/sqrt_big_test.go
#	osmomath/sqrt_test.go
#	osmoutils/osmocli/dynamic_test.go
#	simulation/simtypes/randutil.go
#	tests/cl-genesis-positions/go.mod
#	tests/cl-genesis-positions/go.sum
#	tests/e2e/configurer/chain/chain.go
#	tests/e2e/configurer/chain/commands.go
#	tests/e2e/configurer/chain/queries.go
#	tests/e2e/configurer/upgrade.go
#	tests/e2e/containers/containers.go
#	tests/e2e/e2e_test.go
#	tests/e2e/helpers_e2e_test.go
#	tests/e2e/initialization/config.go
#	tests/e2e/initialization/node.go
#	tests/ibc-hooks/ibc_middleware_test.go
#	tests/ibc-hooks/path_validation_test.go
#	tests/ibc-hooks/xcs_cw20_test.go
#	tests/simulator/sim_test.go
#	tests/simulator/state.go
#	wasmbinding/query_plugin_test.go
#	wasmbinding/test/custom_msg_test.go
#	wasmbinding/test/helpers_test.go
#	wasmbinding/test/messages_test.go
#	x/concentrated-liquidity/bench_test.go
#	x/concentrated-liquidity/client/query_proto_wrap.go
#	x/concentrated-liquidity/event.go
#	x/concentrated-liquidity/incentives.go
#	x/concentrated-liquidity/incentives_test.go
#	x/concentrated-liquidity/math/precompute.go
#	x/concentrated-liquidity/model/msgs.go
#	x/concentrated-liquidity/model/msgs_test.go
#	x/concentrated-liquidity/msg_server.go
#	x/concentrated-liquidity/msg_server_test.go
#	x/concentrated-liquidity/simulation/sim_msgs.go
#	x/concentrated-liquidity/total_liquidity.go
#	x/concentrated-liquidity/types/expected_keepers.go
#	x/concentrated-liquidity/types/gov_test.go
#	x/concentrated-liquidity/types/msgs_test.go
#	x/concentrated-liquidity/types/params_test.go
#	x/cosmwasmpool/cosmwasm/msg/transmuter/transmuter_test.go
#	x/cosmwasmpool/model/pool_test.go
#	x/cosmwasmpool/model/store_model.go
#	x/cosmwasmpool/pool_module_test.go
#	x/gamm/client/cli/query_test.go
#	x/gamm/client/cli/tx_test.go
#	x/gamm/keeper/export_test.go
#	x/gamm/keeper/gas_test.go
#	x/gamm/keeper/genesis_test.go
#	x/gamm/keeper/grpc_query.go
#	x/gamm/keeper/grpc_query_test.go
#	x/gamm/keeper/keeper_test.go
#	x/gamm/keeper/msg_server_test.go
#	x/gamm/keeper/pool.go
#	x/gamm/keeper/pool_test.go
#	x/gamm/keeper/share.go
#	x/gamm/keeper/swap.go
#	x/gamm/keeper/swap_test.go
#	x/gamm/keeper/total_liquidity.go
#	x/gamm/pool-models/balancer/amm_test.go
#	x/gamm/pool-models/balancer/marshal_test.go
#	x/gamm/pool-models/balancer/msgs_test.go
#	x/gamm/pool-models/balancer/pool.go
#	x/gamm/pool-models/balancer/pool_asset.go
#	x/gamm/pool-models/balancer/pool_params.go
#	x/gamm/pool-models/internal/cfmm_common/lp_test.go
#	x/gamm/pool-models/stableswap/msgs_test.go
#	x/gamm/pool-models/stableswap/pool_params.go
#	x/gamm/pool-models/stableswap/util_test.go
#	x/gamm/simulation/sim_msgs.go
#	x/gamm/types/expected_keepers.go
#	x/gamm/types/gov.go
#	x/gamm/types/msgs_test.go
#	x/gamm/types/pool.go
#	x/ibc-rate-limit/ibc_middleware_test.go
#	x/incentives/client/cli/query_test.go
#	x/incentives/keeper/distribute.go
#	x/incentives/keeper/distribute_test.go
#	x/incentives/keeper/export_test.go
#	x/incentives/keeper/gauge.go
#	x/incentives/keeper/gauge_test.go
#	x/incentives/keeper/grpc_query_test.go
#	x/incentives/keeper/keeper_test.go
#	x/incentives/keeper/msg_server_test.go
#	x/incentives/keeper/store_test.go
#	x/incentives/simulation/operations.go
#	x/incentives/types/expected_keepers.go
#	x/incentives/types/gauge.go
#	x/incentives/types/msgs_test.go
#	x/lockup/keeper/bench_test.go
#	x/lockup/keeper/genesis_test.go
#	x/lockup/keeper/grpc_query_test.go
#	x/lockup/keeper/invariants.go
#	x/lockup/keeper/lock.go
#	x/lockup/keeper/lock_test.go
#	x/lockup/keeper/msg_server_test.go
#	x/lockup/keeper/store.go
#	x/lockup/types/msgs_test.go
#	x/mint/keeper/export_test.go
#	x/mint/keeper/genesis.go
#	x/mint/simulation/decoder_test.go
#	x/mint/simulation/genesis.go
#	x/mint/simulation/genesis_test.go
#	x/mint/types/minter_test.go
#	x/mint/types/params_test.go
#	x/pool-incentives/keeper/distr_test.go
#	x/pool-incentives/keeper/genesis.go
#	x/pool-incentives/keeper/genesis_test.go
#	x/pool-incentives/keeper/grpc_query.go
#	x/pool-incentives/keeper/grpc_query_test.go
#	x/pool-incentives/keeper/hooks.go
#	x/pool-incentives/keeper/keeper_test.go
#	x/pool-incentives/types/genesis_test.go
#	x/pool-incentives/types/gov_test.go
#	x/pool-incentives/types/incentives_test.go
#	x/pool-incentives/types/record_test.go
#	x/poolmanager/client/cli/tx.go
#	x/poolmanager/client/cli/tx_test.go
#	x/poolmanager/client/testutil/test_helpers.go
#	x/poolmanager/create_pool_test.go
#	x/poolmanager/events/emit_test.go
#	x/poolmanager/export_test.go
#	x/poolmanager/keeper_test.go
#	x/poolmanager/msg_server_test.go
#	x/poolmanager/router.go
#	x/poolmanager/router_test.go
#	x/poolmanager/taker_fee.go
#	x/poolmanager/types/expected_keepers.go
#	x/poolmanager/types/gov_test.go
#	x/poolmanager/types/msgs_test.go
#	x/poolmanager/types/params.go
#	x/protorev/client/cli/utils.go
#	x/protorev/keeper/developer_fees.go
#	x/protorev/keeper/developer_fees_test.go
#	x/protorev/keeper/emit.go
#	x/protorev/keeper/emit_test.go
#	x/protorev/keeper/epoch_hook_test.go
#	x/protorev/keeper/grpc_query_test.go
#	x/protorev/keeper/hooks.go
#	x/protorev/keeper/hooks_test.go
#	x/protorev/keeper/keeper_test.go
#	x/protorev/keeper/msg_server_test.go
#	x/protorev/keeper/posthandler_test.go
#	x/protorev/keeper/protorev_test.go
#	x/protorev/keeper/rebalance.go
#	x/protorev/keeper/rebalance_test.go
#	x/protorev/keeper/routes.go
#	x/protorev/keeper/routes_test.go
#	x/protorev/keeper/statistics.go
#	x/protorev/keeper/statistics_test.go
#	x/protorev/types/expected_keepers.go
#	x/protorev/types/msg_test.go
#	x/superfluid/client/cli/query_test.go
#	x/superfluid/keeper/concentrated_liquidity.go
#	x/superfluid/keeper/edge_case_test.go
#	x/superfluid/keeper/epoch.go
#	x/superfluid/keeper/epoch_test.go
#	x/superfluid/keeper/export_test.go
#	x/superfluid/keeper/genesis_test.go
#	x/superfluid/keeper/gov/gov_test.go
#	x/superfluid/keeper/grpc_query.go
#	x/superfluid/keeper/hooks.go
#	x/superfluid/keeper/hooks_test.go
#	x/superfluid/keeper/intermediary_account_test.go
#	x/superfluid/keeper/internal/events/emit_test.go
#	x/superfluid/keeper/invariants.go
#	x/superfluid/keeper/keeper_test.go
#	x/superfluid/keeper/msg_server_test.go
#	x/superfluid/keeper/stake_test.go
#	x/superfluid/keeper/superfluid_asset_test.go
#	x/superfluid/keeper/twap_price.go
#	x/superfluid/keeper/twap_price_test.go
#	x/superfluid/keeper/unpool_test.go
#	x/superfluid/simulation/genesis.go
#	x/superfluid/types/errors.go
#	x/superfluid/types/expected_keepers.go
#	x/superfluid/types/msg_test.go
#	x/tokenfactory/keeper/createdenom_test.go
#	x/tokenfactory/module.go
#	x/tokenfactory/types/msgs_test.go
#	x/twap/api.go
#	x/twap/client/query_proto_wrap_test.go
#	x/twap/listeners.go
#	x/twap/migrate_test.go
#	x/twap/store_test.go
#	x/twap/types/twapmock/amminterface.go
#	x/txfees/keeper/feedecorator.go
#	x/txfees/keeper/feedecorator_test.go
#	x/txfees/keeper/feetokens.go
#	x/txfees/keeper/hooks.go
#	x/txfees/keeper/hooks_test.go
#	x/txfees/keeper/keeper_test.go
#	x/txfees/types/expected_keepers.go
#	x/valset-pref/client/cli/query_test.go
#	x/valset-pref/export_test.go
#	x/valset-pref/keeper.go
#	x/valset-pref/keeper_test.go
#	x/valset-pref/msg_server_test.go
#	x/valset-pref/simulation/sim_msgs.go
#	x/valset-pref/types/expected_interfaces.go
#	x/valset-pref/types/msgs_test.go
#	x/valset-pref/validator_set_test.go
@p0mvn p0mvn added the A:backport/v19.x backport patches to v19.x branch label Sep 7, 2023
mergify bot pushed a commit that referenced this pull request Sep 7, 2023
* refactor(deps): switch to cosmossdk.io/math from fork math

* switch remaining things

* updates

(cherry picked from commit ca75f4c)

# Conflicts:
#	go.mod
#	go.sum
#	tests/e2e/e2e_test.go
#	tests/ibc-hooks/ibc_middleware_test.go
#	tests/ibc-hooks/path_validation_test.go
#	x/concentrated-liquidity/incentives.go
#	x/concentrated-liquidity/incentives_test.go
#	x/concentrated-liquidity/math/precompute.go
#	x/concentrated-liquidity/math/tick.go
#	x/concentrated-liquidity/math/tick_test.go
#	x/concentrated-liquidity/model/pool.go
#	x/concentrated-liquidity/pool.go
#	x/concentrated-liquidity/types/constants.go
#	x/concentrated-liquidity/types/errors.go
#	x/incentives/keeper/distribute.go
#	x/incentives/keeper/distribute_test.go
#	x/incentives/keeper/gauge_test.go
#	x/incentives/keeper/keeper_test.go
#	x/incentives/keeper/store_test.go
#	x/poolmanager/router.go
#	x/poolmanager/router_test.go
#	x/protorev/keeper/keeper_test.go
#	x/protorev/keeper/posthandler_test.go
#	x/protorev/keeper/routes_test.go
p0mvn added a commit that referenced this pull request Sep 19, 2023
…6238) (#6341)

* squash

* Generated protofile changes

---------

Co-authored-by: roman <roman@osmosis.team>
Co-authored-by: github-actions <github-actions@github.com>
@p0mvn p0mvn mentioned this pull request Sep 20, 2023
6 tasks
@github-actions github-actions bot mentioned this pull request Apr 15, 2024
@github-actions github-actions bot mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v18.x backport patches to v18.x branch A:backport/v19.x backport patches to v19.x branch C:app-wiring Changes to the app folder C:CLI C:docs Improvements or additions to documentation C:simulator Edits simulator or simulations C:x/concentrated-liquidity C:x/epochs C:x/gamm Changes, features and bugs related to the gamm module. C:x/incentives C:x/lockup C:x/mint C:x/pool-incentives C:x/poolmanager C:x/superfluid C:x/tokenfactory C:x/twap Changes to the twap module C:x/txfees V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants