perf(protorev): base denom as param #7508
Conversation
czarcas7ic
changed the title
czarcas7ic
changed the title
czarcas7ic
requested review from
nicolaslara,
p0mvn and
AlpinYukseloglu
as code owners
|
Wait this wasn't a param before right? How would base denoms get set? I think if we had old logic not going through the param store, we should prefer that |
|
On first pass, this LGTM except the move to using params. Lets stay using a direct state entry as before, just lets write Param usage is always slower than native, and deprecated anyways. |
| // DEPRECATED: The pool weights that are being used to calculate the weight | ||
| // (compute cost) of each route. This field is deprecated and will be removed | ||
| // in the next release. It is replaced by the `info_by_pool_type` field. |
There was a problem hiding this comment.
Drive by change, Skip team agreed this should be marked as deprecated.
There was a problem hiding this comment.
LGTM, though I'm confused why we don't just mark this as reserved? Whats the point of deprecating?
There was a problem hiding this comment.
No you are right we can reserve, I can make the change now
There was a problem hiding this comment.
Reserved here a838fc1
| // Delete the old base denoms | ||
| m.k.DeleteBaseDenoms(ctx) |
There was a problem hiding this comment.
We no longer need to delete the base denoms since we overwrite the single BaseDenoms value.
|
|
||
| // KeyPrefixBaseDenoms is the prefix that is used to store the base denoms that are used to create cyclic arbitrage routes | ||
| KeyPrefixBaseDenoms = []byte{prefixBaseDenoms} |
There was a problem hiding this comment.
I could have probably just used the same old key, but imo after dealing with some of the tests (specifically the upgrade tests) it seemed prudent to just create a new key.
|
This PR is R4R again, Devs requested changes have been made |
czarcas7ic
added
V:state/breaking
Closes: #7495
What is the purpose of the change
Protorev currently requires iterators when retrieving baseDenoms, which is uncached and requires multiple IAVL reads.
This PR implements baseDenoms as a single state entry. This keeps the value in-memory, reducing overhead on sync speed.
Testing and Verifying
Old tests pass with the same values as when the KVStore iterator was used.
Upgrade handler test added and passes.
Full E2E testing should be done on state exported testnet closer to upgrade time.
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)