fix: Allow single-quotes for arbitrage-min-gas-fee #8056
Conversation
WalkthroughThe recent changes in version Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
sigv
force-pushed
the
single-quote-arbitrage-min-gas-fee
branch
2 times, most recently
from
79a382b to
42bfe66
Compare
There is mempool configuration in app.toml, which includes an
arbitrage minimum gas fee value. During startup, Osmosis attempts
to overwrite the app.toml values file. This is done with a direct
string replacement - cutting based on where the nearest quotes are.
Generally this works fine, if the TOML file has standard double
quotes for regular strings. However, if the TOML file has been
overwritten already with tooling that uses literal strings, then
the file will consist of single-quotes instead, as follows:
[osmosis-mempool]
arbitrage-min-gas-fee = '.005'
Such configuration will fail to find double-quotes and use an index
of -1 for the replacement cutting, resulting in an app.toml:
[osmosis-mempool]
0.1
arbitrage-min-gas-fee = '.005'
This results in a failure to restart the Osmosis binary, as the
next startup tries to parse the broken TOML file.
This could be confusing, if later in the file double-quotes are
actually still used, as Osmosis binary would do the replacement
there instead. Such as:
[osmosis-mempool]
arbitrage-min-gas-fee = '.005'
max-gas-wanted-per-tx = "60000000"
Becoming:
[osmosis-mempool]
arbitrage-min-gas-fee = '.005'
max-gas-wanted-per-tx = "0.1"
This commit simply looks for whether a single-quote or a double-quote
is the left-most matched character, before doing a replacement
operation. This ensures that even TOML files with mixed quotes
are handled properly.
This commit does not implement processing of multi-line strings
with `"""` and `'''` boundaries.
sigv
force-pushed
the
single-quote-arbitrage-min-gas-fee
branch
from
42bfe66 to
ac6074d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Actionable comments outside the diff hunks (4)
CHANGELOG.md (4)
Line range hint
986-1064: Correct the heading levels to increment by one level at a time. Replace h4 headings with h3 where applicable.- #### + ###
Line range hint
325-325: Remove the trailing punctuation in the heading.- Initial Release! + Initial Release
Line range hint
588-646: Replace bare URLs with proper markdown links to improve readability and accessibility.- https://github.com/osmosis-labs/osmosis/releases/tag/v3.1.0 + [v3.1.0](https://github.com/osmosis-labs/osmosis/releases/tag/v3.1.0) - https://github.com/osmosis-labs/osmosis/releases/tag/v1.0.1 + [v1.0.1](https://github.com/osmosis-labs/osmosis/releases/tag/v1.0.1)
Line range hint
1227-1227: Ensure that all links have appropriate text descriptions for accessibility and clarity.- [v1.0.0]() + [v1.0.0](https://github.com/osmosis-labs/osmosis/releases/tag/v1.0.0)
There was a problem hiding this comment.
Actionable comments posted: 1
Actionable comments outside the diff hunks (4)
CHANGELOG.md (4)
Line range hint
325-325: Remove trailing punctuation in heading for consistency with other headings.- ## v23.0.12-iavl-v1. + ## v23.0.12-iavl-v1
Line range hint
588-588: Convert bare URL to a markdown link for better readability and to avoid direct exposure of raw URLs.- * [#7994](https://github.com/osmosis-labs/osmosis/pull/7994) Async pruning for IAVL v1 + * [#7994](https://github.com/osmosis-labs/osmosis/pull/7994)[Async pruning for IAVL v1](https://github.com/osmosis-labs/osmosis/pull/7994)
Line range hint
646-646: Convert bare URL to a markdown link for consistency and improved presentation.- * [#525](https://github.com/osmosis-labs/cosmos-sdk/pull/525) CacheKV speedups + * [#525](https://github.com/osmosis-labs/cosmos-sdk/pull/525)[CacheKV speedups](https://github.com/osmosis-labs/cosmos-sdk/pull/525)
Line range hint
1227-1227: Ensure there are no empty links in the document to maintain the integrity and usefulness of the links.- * [Link description]() + * [Link description](https://example.com) # Replace with actual URL
|
@mattverse, thank you I do not think backporting is needed. Could a How should I proceed with the e2e tests failing to auth with Docker Hub? |
mattverse
added
V:state/compatible/backport
|
@sigv I actually think it'd be nice if we can backport this to v24.x, added the label. I'll rerun the CI for you for e2e |
|
Hey @sigv! Really appreciate the PR. I took some time to look into this though and I think this needs to just be refactored (we shouldn't be relying on searching the file and instead should just be unmarshaling it into the proper struct) The refactor I did here solves the issue you pointed out, some other lingering issues, as well as makes it more readable. Again, appreciate the PR! Just think this code was in need of change for a while now. |
|
Confirmed #8118 fixes this along with abstracts the logic to make future config overwrites much easier. Thanks again for the PR to highlight the issue! |
Closes: #8055
What is the purpose of the change
There is mempool configuration in app.toml, which includes an arbitrage minimum gas fee value. During startup, Osmosis attempts to overwrite the app.toml values file. This is done with a direct string replacement - cutting based on where the nearest quotes are. Generally this works fine, if the TOML file has standard double quotes for regular strings. However, if the TOML file has been overwritten already with tooling that uses literal strings, then the file will consist of single-quotes instead, as follows:
Such configuration will fail to find double-quotes and use an index of -1 for the replacement cutting, resulting in an app.toml:
This results in a failure to restart the Osmosis binary, as the next startup tries to parse the broken TOML file.
This could be confusing, if later in the file double-quotes are actually still used, as Osmosis binary would do the replacement there instead. Such as:
Becoming:
This commit simply looks for whether a single-quote or a double-quote is the left-most matched character, before doing a replacement operation. This ensures that even TOML files with mixed quotes are handled properly.
This commit does not implement processing of multi-line strings with
"""and'''boundaries.Testing and Verifying
This change is a
trivial rework/ code cleanup without any test coverage.Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)Summary by CodeRabbit
arbitrage-min-gas-fee.overwriteAppTomlValuesfunction.