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

feat: multi-network upgrade handler #5959

Merged
merged 48 commits into from
Aug 10, 2023
Merged

Conversation

czarcas7ic
Copy link
Member

@czarcas7ic czarcas7ic commented Aug 4, 2023

Closes: #XXX

What is the purpose of the change

This PR introduces logic to the upgrade handler to work with both osmosis-1 and any other network-id (but specifically for osmo-test-5).

If the network ID matches the mainnet ID, osmo-test-a, or osmo-test-b, we create CL pools for all balancer pools defined in the asset pairs struct. Note, we use osmo-test-a and osmo-test-b for E2E, so we want to use the same upgrade handler that mainnet uses whenever possible.

If the network ID is something else, we create CL pools for all existing balancer pools.

From that point, the logic is shared.

A majority of the logic in the PR was pre-existing, but was extracted from the CreateUpgradeHandler method to make it easier to parse.

I did, however, make a core logic change to the upgrade handler where, instead of expecting the baseAsset to be in the community pool already, we swap 0.1 OSMO for the base asset, which is enough to create a full range position.

Testing and Verifying

The upgrade_test.go file was updated to test both "osmosis-1" and "osmo-test-5" network ID.

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

@czarcas7ic czarcas7ic added V:state/breaking State machine breaking PR A:no-changelog labels Aug 4, 2023
@github-actions github-actions bot added the C:app-wiring Changes to the app folder label Aug 4, 2023
@czarcas7ic czarcas7ic marked this pull request as ready for review August 4, 2023 18:41
Comment on lines 146 to 158
wg.Add(4)

go func() {
defer wg.Done()
chainANode.FundCommunityPool(initialization.ValidatorWalletName, strAllUpgradeBaseDenoms())
}()

go func() {
defer wg.Done()
chainBNode.FundCommunityPool(initialization.ValidatorWalletName, strAllUpgradeBaseDenoms())
}()
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to reviewer, we no longer need to fund the community pool with every baseAsset because we now swap osmo from the community pool for the baseAsset!

@@ -24,7 +24,7 @@ const (
// It should be uploaded to Docker Hub. OSMOSIS_E2E_SKIP_UPGRADE should be unset
// for this functionality to be used.
previousVersionOsmoRepository = "osmolabs/osmosis-dev"
previousVersionOsmoTag = "16.0"
previousVersionOsmoTag = "16.0-alpine"
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to reviewer: I am tired of not being able to run a shell within our docker containers, so I changed the image

@@ -689,7 +689,7 @@ func (n *NodeConfig) SendIBC(dstChain *Config, recipient string, token sdk.Coin)
return false
}
},
time.Minute,
3*time.Minute,
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to reviewer: This is a drive by "fix". It became apparent to me that some packets get stuck, so a minute at times in not enough to wait for the result on the receiving chain.

@ValarDragon
Copy link
Member

devbot add changelog misc allow testing with different chain-id's in E2E testing

@ValarDragon
Copy link
Member

devbot merge base;
fix

app/upgrades/v17/upgrades.go Show resolved Hide resolved
app/upgrades/v17/upgrades.go Show resolved Hide resolved
app/upgrades/v17/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/v17/upgrades_test.go Outdated Show resolved Hide resolved
app/upgrades/v17/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/v17/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/v17/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/v17/upgrades.go Show resolved Hide resolved
@ValarDragon
Copy link
Member

devbot help

@devbot-wizard
Copy link
Collaborator

devbot-wizard commented Aug 8, 2023

Hi! I'm DevBot, a bot that helps with common tasks in the development process. Commands:

  • devbot fix: Fix basic errors in the PR. (e.g. imports, changelog conflicts, go mod conflicts)
  • devbot merge base: Merge the base branch into the PR branch.
  • devbot add changelog [misc/feat/api/sec] [message]: Add a changelog entry to the PR. (e.g. devbot add changelog feat Added a new feature)
    • If message is blank, defaults to PR title.
  • devbot re-pr: Re-PR the PR, useful for external contributors where we have no edit perms.

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

e2eChainB = "osmo-test-b"
)

var notEnoughLiquidityForSwapErr = errorsmod.Wrapf(gammtypes.ErrInvalidMathApprox, "token amount must be positive")
Copy link
Member

Choose a reason for hiding this comment

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

nit: why don't we import this error from the place where it is returned in superfluid?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no error to import. Its from gamm, and every instance of this is manually entered. Example:

return sdk.Int{}, errorsmod.Wrapf(types.ErrInvalidMathApprox, "token amount must be positive")
}

return sdk.Coin{}, errorsmod.Wrapf(types.ErrInvalidMathApprox, "token amount must be positive")
}

return sdk.Coin{}, errorsmod.Wrapf(types.ErrInvalidMathApprox, "token amount must be positive")
}

app/upgrades/v17/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/v17/upgrades.go Outdated Show resolved Hide resolved
@czarcas7ic czarcas7ic merged commit a0bfd58 into main Aug 10, 2023
@czarcas7ic czarcas7ic deleted the adam/multi-network-upgrade-handler branch August 10, 2023 00:27
p0mvn added a commit that referenced this pull request Aug 29, 2023
* multi-network upgrade handler

* run mainnet upgrade handler for e2e network IDs

* fixes

* fix

* widen tolerance

* more resilient way to calculate coins used

* add logs to upgrade handler

* scale error tolerance with spread factor

* change back init tag

* update changelog

* Fix imports

* check liquidity prior to creating pool

* check base/quote rather than all denoms

* check if pool only has 2 assets

* skip pools that are already linked

* add log and fix issue

* add an additional log

* more logs

* more prints

* skip if incentive gauge duration does not exist

* use gogotypes

* use store.Has

* error instead of panic on getgaugeId from poolId

* Update app/upgrades/v17/upgrades_test.go

Co-authored-by: Roman <[email protected]>

* remove extra prints

* remove old logs

* enable superfluid assets both chains

* remove unused func

* setting the default ibc-hooks params

* separate method for enable superfluid for testnet

* drastically simplify the upgrade handler

* check superfluid error type directly

* remove use of pointer of coinsUsed and poolLinks

* Update app/upgrades/v17/upgrades.go

* Update app/upgrades/v17/upgrades.go

* add spot price check when adding testnet assets

* fix osmo amount

* add error check

* add additional check from testnet to mainnet

* remove this check from mainnet handler

* fix lingering merge conflict

---------

Co-authored-by: devbot-wizard <[email protected]>
Co-authored-by: Roman <[email protected]>
Co-authored-by: Nicolas Lara <[email protected]>
@github-actions github-actions bot mentioned this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:no-changelog C:app-wiring Changes to the app folder C:x/gamm Changes, features and bugs related to the gamm module. C:x/pool-incentives V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants