Alloy Transmuter Support #245

iboss-ptk · 2024-05-22T03:09:57Z

Add support for alloy transmuter
Use cw2 info to determine the pool type
Pass along CosmWasmPoolModel with information queried from contract to sqs
Fix CosmWasm pool conversion failed on streaming update due to missing wasmkeeper

sqs tag for these changes block osmosis-labs/osmosis#8291

p0mvn

Just did a quick skim without any deep review for now. Looks like everything is going well and on the right track. Great work!

sqsdomain/pools.go

sqs.log

sqsdomain/pools.go

p0mvn

Great work! Left some comments

sqsdomain/cw_pool.go

router/usecase/router.go

router/usecase/pools/pool_factory.go

router/usecase/pools/routable_cw_alloy_transmuter_pool.go

router/usecase/pools/routable_cw_alloy_transmuter_pool_test.go

router/usecase/pools/routable_cw_alloy_transmuter_pool.go

p0mvn · 2024-05-28T13:17:10Z

Also, would be great to document somewhere the logic around how CWPoolModel relies on the contract info and how this is different from transmuter and generalized cw pools. Maybe we could start a doc under docs/architecture/CW_POOLS.MD?

p0mvn · 2024-05-30T09:43:25Z

@iboss-ptk, please ping me once ready for another round of 👀

domain/router.go

router/usecase/pools/routable_cw_alloy_transmuter_pool.go

router/usecase/pools/routable_cw_alloy_transmuter_pool_test.go

router/usecase/pools/routable_cw_alloy_transmuter_pool.go

p0mvn

This already looks good to me. Nice work!

However, I would like to run the tests before approving. Will come back to this.

Btw please make PRs from this repo directly (not fork) - it would simplify the review process in the future.

I will add you to the repo

p0mvn · 2024-05-31T13:10:48Z

config.json

-        ]
+        ],
+        "is-alloyed-transmuter-enabled": true


Can you please update the following and merge them before this PR is merged?

Template: https://github.com/osmosis-labs/infrastructure/blob/55a5946330250d38047d26f8c4865474436db9d3/environments/sqs-osmosis-zone/ansible/roles/prepare_local_folder/templates/sqs/config.json.j2

Configs:

https://github.com/osmosis-labs/infrastructure/blob/main/environments/sqs-osmosis-zone/environments/stage/ansible/vars/sqs.yaml

https://github.com/osmosis-labs/infrastructure/blob/main/environments/sqs-osmosis-zone/environments/prod/ansible/vars/sqs.yaml

https://github.com/osmosis-labs/infrastructure/blob/main/environments/sqs-osmosis-zone/environments/testnet/ansible/vars/sqs.yaml

I can't access infrastructure repo

router/usecase/pools/routable_cw_alloy_transmuter_pool_test.go

router/usecase/pools/routable_cw_alloy_transmuter_pool.go

sqsdomain/cw_pool.go

p0mvn · 2024-06-03T03:48:00Z

sqsdomain/cw_pool.go

+}
+
+func (model *CosmWasmPoolModel) IsAlloyTransmuter() bool {
+	return model.ContractInfo.Matches("crates.io:transmuter", "3.0.0")


What if the contract is migrated? We would then require code updates and redeployment?

Please consider removing this hard-coded dependency here and on the node side.

The current code ID model allows us to simply update the config without performing any code changes

Please consider going back to the code ID config when determining if a pool is an alloy transmuter within SQS.

To achieve this on the Osmosis node side, we can continue utilizing the CW info but only match the name e.g. crates.io:transmuter-alloy (no version). This way, if the version is updated, it will still be auto-propagated from Osmosis node. However, give SQS the flexibility of handling it via standard means.

The proposed code ID approach would ensure compatibility with how other cw pool types are handled while also allowing the flexibility of only updating configs without code changes if migration is done.

With the current approach, we still need a boolean config but also hardcode the version here which I think is suboptimal for operations.

Please let me know what you think

but then, this logic required on the ingester side, do we also have config on that side, I didn't see any. If there is any update required, both side needs to be effected.

I think versions are more reliable than code ids due to it's characteristics that it does not change from env to env. That's why I think it's ok to hard code it and have it defined once (in sqs repo). Even though the contract gets migrated and it requires no change on sqs. It will still work. These infos are bound to conversion logic anyway.

For versioning matching, we can use semver matching to check on which pool version we are operating on as well to increase range of compatability. For example, for alloy transmuter it can be ">=3.0.0" for now.

Other cosmwasmpool can also adopt this approach (I didn't update then in this PR since I would love to push this out earlier to test it with USDT alloyed, it requires a bit more regression testing).

Bottom line is that, we can't avoid code changes if the logics for calculating token out changes. We can be lax on the config (eg. ">=3.0.0" and no restriction on the upper bound) until there is a version that requires new calculation. We can have sqs disregard newer versions via config if that's a desired property of the system to control incompatible pool type to not get calculated.

On another point, "matching name only" does not work well IMO. Since there might be changes in the pool transformation logic in the future version, which means we have to change contract name again.

For visibility, this is Boss's reply (did not get submitted publicly) that I will address in a separate message

I think that we should discuss this decision via a separate ADR to avoid blocking this line of work.

I would like to avoid merging partial progress with a new paradigm that has trade-offs. If we want to change how CW pools are handled, let's aim to change this for all pools at once rather than introducing custom handling for the alloyed pool but keeping everything else via code IDs.

What I would like to avoid is us never coming back to this and then having to deal with operational challenges stemming from the custom logic.

In terms of versions, I don't understand why we need to track these hard-coded versions in general when contract names should suffice. We should establish a rule that all future versions of a cw pool model struct transferred between node and sqs are backward compatible. This removes the need for version matching on the node side, removing the need to either have a config (node-side) or hard code it in sqsdomain.

I'm supportive of using versions in the SQS config rather than hardcoding. But again, I propose making that decision in a separate ADR and/or issue to avoid overly long debates on the otherwise complete work here.

Again, my main counter-argument to the proposed change in this PR is the following:

Operational challenges stemming from hardcoding versions on both osmosis and sqs side

Applying an operational change to one cw pool only, causing inconsistencies in handling

The trade-offs are not fully analyzed before making a change that impacts service operations

However, I think that we should explore the version idea in SQS configs in general. That is an interesting proposal.

@iboss-ptk please let me know what you think

Fair point on consistency.

I will revert back the config to code ids and and have a separated ADR for versioning we discussed to unblock things. That being said. Since old transmuter is not forward compatible, it still need minimum version >=3.0.0 hardcoded. Because even though it is a non-issue on the sqs side in case there is no minimum version in place, ingester needs this information to do pool transformation properly as old transmuter does not have those queries we use for v3.0.0.

And current transmuter v3 on mainnet is already crates.io:transmuter v3.0.0 so this can't be changed to crates.io:transmuter-alloy unless having a migration (and I don't think that make much sense anyway since it is still a newer version of the same thing).

To summarize:

I don't think we can get rid of any hardcoded version except making more lax and keep backward compatibility. Which might or might not make sense depending on

Thanks

Per this comment, the latest solution is more extensible.

If we keep it as a convention that all alloyed transmuters stay within the same major version and add a relevant semver constraint (<4.0.0), we solve all future extensibility problems.

In the original approach, due to semver being hardcoded directly, we would have to make code changes and dependency updates to all of sqsdomain, osmosis and sqs if a migration was done.

Now, migrations within the same major are auto-accounted for

p0mvn · 2024-06-04T11:47:49Z

sqsdomain/cw_pool.go

+	name := "crates.io:transmuter"
+	version := ">= 3.0.0"
+
+	constraints, err := semver.NewConstraint(version)
+	// this must never panic
+	if err != nil {
+		panic(err)
+	}
+	return model.ContractInfo.Matches(name, constraints)


I like this approach. It is now extensible without having to make code changes to both, the osmosis node side and sqs for minor migrations.

Can we also add constraint of semver being less than <4.0.0 so that, if there are any future versions, those are auto-accounted for without more code changes?

At least for now that I'm aware of, 4.0.0 will still be compatible, so I would avoid constraining that if you don't want code change. Until there is a version the requires changes in logic, I think leave this lax constraints would be easier to operate.

I think we should have a standard where a separate major means a breaking change.

If 4.0.0 is compatible, can it be 3.1.0 instead?

It's breaking other parts. For 4.0.0 we are planning to remove some entrypoints, putting more constraints on denom, but just does not effect sqs.

Got it, let's leave it as is. Thank you!

config-testnet.json

p0mvn · 2024-06-04T11:57:29Z

docs/architecture/COSMWASM_POOLS.MD

@@ -0,0 +1,46 @@
+# CosmWasm Pools


Do you mind adding a paragraph about semver and the general new version handling we arrived at with the cw pools? For example, "alloyed transmuter versions are expected to exist within the same major version i.e >=3.0.0 and <4.0.0"

p0mvn · 2024-06-04T12:01:33Z

@iboss-ptk thanks for addressing all operational concerns. Great work.

I pre-approved the PR but the following things remain:

Update infrastructure repository with the new config values: https://github.com/osmosis-labs/infrastructure
- Please do no merge before this is done because our stage deployments will break
- You should have access. If still not, please ask in #eng-platform-support
Add docs about semver handling

iboss-ptk added 3 commits May 21, 2024 12:37

fix: router error with wrong pool type

17ddd7f

support new cw pool: transmuter with alloyed

702c13d

rename

a3b8884

p0mvn reviewed May 22, 2024

View reviewed changes

sqsdomain/pools.go Outdated Show resolved Hide resolved

sqs.log Outdated Show resolved Hide resolved

sqsdomain/pools.go Outdated Show resolved Hide resolved

iboss-ptk added 8 commits May 27, 2024 15:58

Update proto

adfd6c5

Parse CwPoolModel

c220c0c

Make cw pool that has cw pool model pass validation

1ed353f

Move CwPoolModel into SQSModel

7d08551

rename CwPoolModel to CosmWasmPoolModel

f4d447a

remove sqs.log

95306f5

fix tests

3da4e4b

Merge branch 'v25.x' into boss/transmuter-alloyed

04ecf90

iboss-ptk marked this pull request as ready for review May 28, 2024 08:00

iboss-ptk requested a review from p0mvn May 28, 2024 08:04

p0mvn reviewed May 28, 2024

View reviewed changes

add more comments

a1b7ce4

iboss-ptk added 6 commits May 30, 2024 16:49

add cosmwasm pool doc

2801da0

add feature flag for alloyed transmuter

bf75aae

remove wrong logging

9e1b326

Merge branch 'v25.x' into boss/transmuter-alloyed

542bcf7

add tests for find normalization factor

93b662d

add calc test cases

55992fd

p0mvn reviewed May 30, 2024

View reviewed changes

iboss-ptk added 3 commits May 30, 2024 21:18

format indentation

a0696da

refactor and add test cases

ff63b6b

zero normalization factor error

4e31bd2

iboss-ptk requested a review from p0mvn May 31, 2024 06:43

p0mvn reviewed May 31, 2024

View reviewed changes

p0mvn reviewed Jun 3, 2024

View reviewed changes

iboss-ptk added 5 commits June 3, 2024 14:19

extract constants

e289e29

return empty value on error & shorten math ops

e74080c

Merge branch 'v25.x' into boss/transmuter-alloyed

9d176af

laxing version constraints

950d22d

use code id for configuring alloyed transmuter

a18226e

p0mvn reviewed Jun 4, 2024

View reviewed changes

p0mvn approved these changes Jun 4, 2024

View reviewed changes

Update config-testnet.json

a05cf85

update docs

c4a0fb8

p0mvn merged commit 547e0e1 into osmosis-labs:v25.x Jun 6, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alloy Transmuter Support #245

Alloy Transmuter Support #245

iboss-ptk commented May 22, 2024 •

edited

Loading

p0mvn left a comment

p0mvn left a comment

p0mvn commented May 28, 2024

p0mvn commented May 30, 2024

p0mvn left a comment

p0mvn May 31, 2024

iboss-ptk Jun 3, 2024

p0mvn Jun 3, 2024

p0mvn Jun 3, 2024

iboss-ptk Jun 3, 2024

iboss-ptk Jun 3, 2024

iboss-ptk Jun 3, 2024

p0mvn Jun 3, 2024

p0mvn Jun 3, 2024

iboss-ptk Jun 4, 2024 •

edited

Loading

p0mvn Jun 4, 2024

p0mvn Jun 4, 2024

iboss-ptk Jun 5, 2024

p0mvn Jun 6, 2024

iboss-ptk Jun 6, 2024

p0mvn Jun 6, 2024

p0mvn Jun 4, 2024

p0mvn commented Jun 4, 2024 •

edited

Loading

Alloy Transmuter Support #245

Alloy Transmuter Support #245

Conversation

iboss-ptk commented May 22, 2024 • edited Loading

p0mvn left a comment

Choose a reason for hiding this comment

p0mvn left a comment

Choose a reason for hiding this comment

p0mvn commented May 28, 2024

p0mvn commented May 30, 2024

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iboss-ptk Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn commented Jun 4, 2024 • edited Loading

iboss-ptk commented May 22, 2024 •

edited

Loading

iboss-ptk Jun 4, 2024 •

edited

Loading

p0mvn commented Jun 4, 2024 •

edited

Loading