fix: LegacyDec transactions #3868
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
WalkthroughThis pull request introduces a suite of unit tests for the Changes
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (38)
packages/server/src/queries/osmosis/concentratedliquidity/params.ts (2)
3-14: LGTM: Well-structured type definition. Consider minor improvement for consistency.The
ClParamsResponsetype is comprehensive and aligns well with the PR objectives. It covers essential parameters for concentrated liquidity, including authorized tick spacing, spread factors, and pool creation settings.For consistency, consider using plural forms for all array properties. Apply this change:
export type ClParamsResponse = { params: { authorized_tick_spacing: string[]; authorized_spread_factors: string[]; balancer_shares_reward_discount: string; authorized_quote_denoms: string[]; authorized_uptimes: string[]; is_permissionless_pool_creation_enabled: boolean; - unrestricted_pool_creator_whitelist: string[]; + unrestricted_pool_creators_whitelist: string[]; hook_gas_limit: string; }; };
16-18: LGTM: Correctly implemented query constant. Consider minor readability improvement.The
queryClParamsconstant is properly implemented usingcreateNodeQueryand correctly utilizes theClParamsResponsetype. The API path is appropriate for querying concentrated liquidity parameters.For improved readability, consider extracting the API path to a named constant:
+const CL_PARAMS_API_PATH = "/osmosis/concentratedliquidity/v1beta1/params"; + export const queryClParams = createNodeQuery<ClParamsResponse>({ - path: "/osmosis/concentratedliquidity/v1beta1/params", + path: CL_PARAMS_API_PATH, });This change enhances maintainability and makes it easier to update the path if needed in the future.
packages/proto-codecs/src/codegen/index.ts (1)
1-1: Consider removing the@ts-nocheckdirectiveWhile not a change in this PR, it's worth noting that the file uses the
@ts-nocheckdirective. This suppresses all TypeScript errors in the file, which might be intentional for generated code but generally isn't a best practice for maintainable TypeScript.Consider evaluating if this directive is necessary or if it can be replaced with more specific type assertions or suppressions.
packages/proto-codecs/scripts/__tests__/decimal-patch.spec.ts (5)
1-9: Excellent approach for testing multiple implementations.The setup to test both
CodegenDecimalandDecimalPatchimplementations with the same test cases is a great practice. It ensures consistency and makes it easier to maintain the tests.Consider adding a type annotation to the
Decimalparameter in theforEachloop for improved code clarity:[DecimalPatch, CodegenDecimal].forEach((Decimal: typeof DecimalPatch | typeof CodegenDecimal) => { // ... test cases ... });
16-26: Comprehensive error handling test for invalid user input.This test case effectively covers multiple error scenarios for invalid user input, which is crucial for robust input validation. The use of separate expect statements for each scenario improves test readability and helps pinpoint specific failures.
For consistency, consider using template literals for all error messages:
expect(() => Decimal.fromUserInput("123.45.6", 3)).toThrow( "More than one separator found" ); expect(() => Decimal.fromUserInput("123a.456", 3)).toThrow( `Invalid character at position 4` ); expect(() => Decimal.fromUserInput("123.4567", 3)).toThrow( `Got more fractional digits than supported` );
28-40: Good coverage of Decimal creation from atomics and string conversion.These test cases effectively validate the creation of Decimal instances from atomic values and the conversion of Decimal instances to strings. The inclusion of a case with no fractional part is a good touch.
Consider adding a test case for creating a Decimal from atomics with a large number of fractional digits to ensure proper handling of precision:
test("should handle large number of fractional digits", () => { const decimal = Decimal.fromAtomics("123456789", 9); expect(decimal.toString()).toBe("0.123456789"); });
42-48: Good coverage of edge cases.This test case effectively checks the handling of zero values and maximum fractional digits, which are important edge cases for decimal arithmetic.
Consider adding a test for another edge case: the maximum possible value that can be represented by the Decimal class. This would help ensure that the class correctly handles values at the upper limit of its range:
test("should handle maximum possible value", () => { const maxValue = "340282366920938463463.374607431768211455"; // Example max value const decimal = Decimal.fromUserInput(maxValue, 18); expect(decimal.toString()).toBe(maxValue.replace(".", "")); });
50-54: Good test for handling small fractions with high precision.This test case effectively verifies the handling of small fractional values with a large number of fractional digits (18), which is particularly relevant in many blockchain contexts.
To further improve coverage, consider adding a test case for rounding behavior when the number of fractional digits exceeds the specified precision:
test("should correctly round when exceeding fractional digits", () => { const decimal = Decimal.fromUserInput("0.1234567890123456789", 18); expect(decimal.toString()).toBe("123456789012345678"); });packages/proto-codecs/scripts/codegen.ts (1)
110-122: Approve patching process with suggestions for improvement.The implementation of the patching process for the Decimal class is a good approach to ensure the custom implementation is used. However, consider the following suggestions to improve robustness:
- Add error handling for file operations.
- Use asynchronous file operations for better performance.
- Add a check to ensure the patch file exists before attempting to read it.
Here's an improved version of the patching process:
const patchDecimalsFile = async () => { const patchFilePath = join(__dirname, "./decimals-patch.ts"); const targetFilePath = join(__dirname, "../src/codegen/decimals.ts"); try { if (!fs.existsSync(patchFilePath)) { throw new Error("Patch file does not exist"); } const decimalsPatchContent = await fs.promises.readFile(patchFilePath, "utf8"); await fs.promises.writeFile(targetFilePath, decimalsPatchContent); console.info("✅ Decimals file patched successfully"); } catch (error) { console.error("Error patching decimals file:", error); process.exit(1); } }; // ... rest of the code ... telescope(/* ... */) .then(patchDecimalsFile) .then(() => { console.info("✨ all done!"); }) .catch((e) => { console.error(e); process.exit(1); });This improved version includes error handling, uses asynchronous file operations, and checks for the existence of the patch file before attempting to read it.
packages/proto-codecs/src/codegen/osmosis/superfluid/params.ts (1)
Line range hint
1-124: Summary of changes and potential impactsThe main change in this file is the switch from an external
Decimalimplementation to a local one. While the usage of theDecimalclass remains largely the same, this change could have significant impacts on the superfluid module, particularly in how decimal values are handled.Key points to consider:
- Ensure that the new
Decimalimplementation is fully tested and compatible with the rest of the codebase.- Verify that the precision and rounding behavior remain consistent, especially for the
minimumRiskFactorparameter.- Consider the broader implications of this change on the superfluid module and any dependent systems.
Given that this change is likely part of a larger effort to use a custom
Decimalimplementation, consider the following:
- Document the reasons for this change and any differences in behavior compared to the previous implementation.
- Update any relevant documentation or specifications that reference decimal handling in the superfluid module.
- Consider creating a migration guide if this change affects how external systems interact with the superfluid module.
Would you like assistance in creating a comprehensive test suite for the new
Decimalimplementation and its usage in the superfluid module?packages/web/components/complex/pool/create/cl-pool.tsx (1)
131-135: LGTM: Hardcoded strings replaced with translation function calls.The changes successfully implement internationalization in the
getStepHeaderfunction:
- Hardcoded strings are replaced with appropriate translation keys.
- The dynamic
poolNumberparameter is correctly passed to the translation function.For consistency, consider using template literals for the translation key in the second case:
- return t("pools.createSupercharged.addInitialLiquidity", { + return t(`pools.createSupercharged.addInitialLiquidity`, { poolNumber: poolNumber ?? "", });This change would make the syntax consistent with the first case and improve readability.
packages/stores/src/account/amino-converters.ts (1)
Line range hint
1-134: Summary of changes and recommendationsThe modifications in this file primarily focus on updating amino converters for specific message types, which aligns well with the PR objectives. The changes address issues related to Decimal changes in Osmosis version 26 and contribute to code cleanliness by removing unused converters and simplifying existing ones.
Key points:
- The addition of the
MsgCreateConcentratedPoolconverter supports the creation of all types of pools.- The removal of unused converters, such as
MsgWithdrawPosition, simplifies the codebase.- There's a potential inconsistency in the IBC transfer converter (
MsgTransfer) that needs attention.Recommendations:
- Address the inconsistency in the
MsgTransferconverter by aligning thefromAminomethod with thetoAminomethod regarding thetimeout_timestampfield.- Conduct thorough testing to ensure that these changes don't negatively impact the functionality of swaps, as mentioned in the PR objectives.
Consider creating a comprehensive test suite that covers all the modified converters to ensure their correct functionality and to prevent future regressions.
packages/proto-codecs/src/codegen/osmosis/valsetpref/v1beta1/state.ts (2)
Line range hint
101-104: Approve weight encoding with a suggestion for clarityThe use of
Decimal.fromUserInputto convert the weight to atomics before encoding is a good practice. It ensures that the weight is stored with high precision in the encoded data.Consider adding a comment explaining the conversion process for better clarity:
if (message.weight !== "") { + // Convert weight to atomics for high-precision storage writer .uint32(18) .string(Decimal.fromUserInput(message.weight, 18).atomics); }
Line range hint
124-126: Approve weight decoding with a suggestion for error handlingThe use of
Decimal.fromAtomicsto convert the weight from atomics back to a string representation is correct and maintains consistency with the encoding process.Consider adding error handling to manage potential issues during the conversion:
case 2: - message.weight = Decimal.fromAtomics(reader.string(), 18).toString(); + try { + message.weight = Decimal.fromAtomics(reader.string(), 18).toString(); + } catch (error) { + console.error("Error decoding weight:", error); + message.weight = "0"; // or another appropriate default value + } break;This change will make the decoding process more robust by handling potential errors during the conversion.
packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/poolmodel/concentrated/v1beta1/tx.ts (2)
Line range hint
93-97: LGTM. Consider adding a comment for clarity.The change to use
Decimal.fromUserInput(message.spreadFactor, 18).atomicsfor encoding thespreadFactoris a good improvement. It standardizes the encoding of decimal values, which aligns with the PR objectives.Consider adding a brief comment explaining the significance of the 18 decimal places precision:
if (message.spreadFactor !== "") { writer .uint32(42) + // Convert to atomics with 18 decimal places precision .string(Decimal.fromUserInput(message.spreadFactor, 18).atomics); }
Line range hint
124-128: LGTM. Consider consistent variable naming.The change to use
Decimal.fromAtomics(reader.string(), 18).toString()for decoding thespreadFactoris a good improvement. It ensures consistent handling of decimal values throughout the transaction process.For consistency with the encoding method, consider extracting the precision value to a constant:
+const SPREAD_FACTOR_PRECISION = 18; // In the encode method -.string(Decimal.fromUserInput(message.spreadFactor, 18).atomics); +.string(Decimal.fromUserInput(message.spreadFactor, SPREAD_FACTOR_PRECISION).atomics); // In the decode method -message.spreadFactor = Decimal.fromAtomics(reader.string(), 18).toString(); +message.spreadFactor = Decimal.fromAtomics(reader.string(), SPREAD_FACTOR_PRECISION).toString();This change would make it easier to maintain and update the precision if needed in the future.
packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/v1beta1/incentive_record.ts (2)
Line range hint
270-273: LGTM: Proper conversion of emissionRate to atomic representation.The change to use
Decimal.fromUserInputfor convertingemissionRateto its atomic representation before encoding is correct and aligns with the PR objectives. This ensures accurate handling of high-precision decimal values for emission rates.Consider adding a comment explaining the choice of 18 as the precision value for better code maintainability:
if (message.emissionRate !== "") { writer .uint32(18) - .string(Decimal.fromUserInput(message.emissionRate, 18).atomics); + .string(Decimal.fromUserInput(message.emissionRate, 18).atomics); // 18 decimal places for high precision }
Line range hint
292-295: LGTM: Proper conversion of emissionRate from atomic representation.The change to use
Decimal.fromAtomicsfor convertingemissionRatefrom its atomic representation to a string during decoding is correct and complements the encoding change. This ensures consistent handling of high-precision decimal values for emission rates.For consistency with the encode method and better code maintainability, consider extracting the precision value to a constant:
+const EMISSION_RATE_PRECISION = 18; // In the encode method: - .string(Decimal.fromUserInput(message.emissionRate, 18).atomics); + .string(Decimal.fromUserInput(message.emissionRate, EMISSION_RATE_PRECISION).atomics); // In the decode method: - message.emissionRate = Decimal.fromAtomics(reader.string(), 18).toString(); + message.emissionRate = Decimal.fromAtomics(reader.string(), EMISSION_RATE_PRECISION).toString();packages/proto-codecs/src/codegen/osmosis/gamm/poolmodels/stableswap/v1beta1/stableswap_pool.ts (2)
Line range hint
141-150: LGTM! Consider adding a test case for fee encoding.The modification to use
Decimal.fromUserInputfor convertingswapFeeandexitFeeto their atomic representations before encoding is a good improvement. This change aligns with the PR objective of addressing transaction issues related to Decimal changes.To ensure the correct behavior of this encoding process, consider adding a test case that verifies the encoding of
PoolParamswith various fee values. Would you like assistance in creating this test case?
Line range hint
163-170: LGTM! Consider adding a test case for fee decoding.The modification to use
Decimal.fromAtomicsfor converting the atomic representations ofswapFeeandexitFeeback to string format during decoding is a good improvement. This change complements the encoding modifications and aligns with the PR objective.To ensure the correct behavior of this decoding process, consider adding a test case that verifies the decoding of
PoolParamswith various fee values. Would you like assistance in creating this test case?packages/proto-codecs/src/codegen/osmosis/accum/v1beta1/accum.ts (2)
Line range hint
149-152: Approve the Decimal encoding change with a suggestion for improvement.The change to use
Decimal.fromUserInput(message.totalShares, 18).atomicsfor encodingtotalSharesis a good step towards addressing the Decimal-related transaction issues mentioned in the PR objectives. This ensures that the value is converted to its atomic representation before being written.Consider making the decimal places (currently hardcoded to 18) configurable. This would provide more flexibility for different types of assets or future changes. For example:
const TOTAL_SHARES_DECIMAL_PLACES = 18; // Define this constant at the module level // In the encode method writer.uint32(18).string(Decimal.fromUserInput(message.totalShares, TOTAL_SHARES_DECIMAL_PLACES).atomics);This approach maintains the current behavior while allowing for easier adjustments if needed in the future.
Line range hint
324-327: Approve the Decimal encoding change and ensure consistency.The change to use
Decimal.fromUserInput(message.numShares, 18).atomicsfor encodingnumSharesis consistent with the previous change and addresses the Decimal-related transaction issues mentioned in the PR objectives.For consistency with the previous suggestion, consider using the same configurable approach for decimal places:
const NUM_SHARES_DECIMAL_PLACES = 18; // Define this constant at the module level, or use the same constant as TOTAL_SHARES_DECIMAL_PLACES if they should always be the same // In the encode method writer.uint32(10).string(Decimal.fromUserInput(message.numShares, NUM_SHARES_DECIMAL_PLACES).atomics);This maintains consistency across the codebase and allows for easier future adjustments if needed.
packages/proto-codecs/src/codegen/osmosis/poolmanager/v1beta1/taker_fee_share.ts (1)
Line range hint
193-194: LGTM with a suggestion for improvementThe changes to the
encodeanddecodemethods ofTakerFeeShareAgreementcorrectly implement the new Decimal functionality. The conversion between user input and atomic representation is maintained.Consider making the precision (currently hardcoded to 18 decimal places) configurable or documenting the reason for this specific value. This would improve flexibility and maintainability of the code.
Example of how to make it configurable:
+ const SKIM_PERCENT_PRECISION = 18; - .string(Decimal.fromUserInput(message.skimPercent, 18).atomics); + .string(Decimal.fromUserInput(message.skimPercent, SKIM_PERCENT_PRECISION).atomics); - message.skimPercent = Decimal.fromAtomics(reader.string(), 18).toString(); + message.skimPercent = Decimal.fromAtomics(reader.string(), SKIM_PERCENT_PRECISION).toString();Also applies to: 211-214
packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/v1beta1/position.ts (2)
Line range hint
201-203: Approve liquidity encoding change with a minor suggestionThe change to encode the
liquidityfield usingDecimal.fromUserInput(message.liquidity, 18).atomicsis a good improvement for consistent handling of decimal values.Consider extracting the magic number
18into a named constant for better readability and maintainability. For example:+const LIQUIDITY_DECIMAL_PLACES = 18; if (message.liquidity !== "") { writer .uint32(58) - .string(Decimal.fromUserInput(message.liquidity, 18).atomics); + .string(Decimal.fromUserInput(message.liquidity, LIQUIDITY_DECIMAL_PLACES).atomics); }
Line range hint
236-238: Approve liquidity decoding change with a minor suggestionThe change to decode the
liquidityfield usingDecimal.fromAtomics(reader.string(), 18).toString()is a good improvement for consistent handling of decimal values.For consistency with the encoding method, consider extracting the magic number
18into the same named constant. For example:+const LIQUIDITY_DECIMAL_PLACES = 18; case 7: message.liquidity = Decimal.fromAtomics( reader.string(), - 18 + LIQUIDITY_DECIMAL_PLACES ).toString(); break;packages/proto-codecs/src/codegen/osmosis/gamm/v1beta1/balancerPool.ts (2)
Line range hint
441-448: Approve the change with a suggestion for improvement.The modification to use
Decimal.fromUserInputforswapFeeandexitFeein theencodemethod is a good improvement. It ensures proper decimal representation of the fees.Consider adding a constant for the decimal places (18) to improve code readability and maintainability. For example:
+const FEE_DECIMAL_PLACES = 18; export const PoolParams = { // ... encode( message: PoolParams, writer: BinaryWriter = BinaryWriter.create() ): BinaryWriter { if (message.swapFee !== "") { writer .uint32(10) - .string(Decimal.fromUserInput(message.swapFee, 18).atomics); + .string(Decimal.fromUserInput(message.swapFee, FEE_DECIMAL_PLACES).atomics); } if (message.exitFee !== "") { writer .uint32(18) - .string(Decimal.fromUserInput(message.exitFee, 18).atomics); + .string(Decimal.fromUserInput(message.exitFee, FEE_DECIMAL_PLACES).atomics); } // ... }, // ... };
Line range hint
461-467: Approve the change with a suggestion for improvement.The modification to use
Decimal.fromAtomicsforswapFeeandexitFeein thedecodemethod is a good improvement. It ensures proper conversion from atomic representation to user-friendly decimal representation.Consider using the same constant for decimal places as suggested in the
encodemethod to improve consistency and maintainability:+const FEE_DECIMAL_PLACES = 18; export const PoolParams = { // ... decode(input: BinaryReader | Uint8Array, length?: number): PoolParams { // ... switch (tag >>> 3) { case 1: - message.swapFee = Decimal.fromAtomics(reader.string(), 18).toString(); + message.swapFee = Decimal.fromAtomics(reader.string(), FEE_DECIMAL_PLACES).toString(); break; case 2: - message.exitFee = Decimal.fromAtomics(reader.string(), 18).toString(); + message.exitFee = Decimal.fromAtomics(reader.string(), FEE_DECIMAL_PLACES).toString(); break; // ... } // ... }, // ... };packages/proto-codecs/src/codegen/osmosis/cosmwasmpool/v1beta1/model/module_sudo_msg.ts (1)
Line range hint
1-724: Summary of changes and potential impact.The main change in this file is the adoption of a new
Decimalimplementation from a local source instead of@cosmjs/math. This modification affects the encoding ofswapFeein bothSwapExactAmountInandSwapExactAmountOutinterfaces. While the changes appear to be consistent and well-implemented, it's crucial to ensure that:
- The new
Decimalimplementation is fully compatible with the rest of the codebase.- There are no precision issues introduced by this change.
- All parts of the system that interact with these swap functions are updated to handle the potentially different
Decimalrepresentation.These changes are likely part of a broader effort to standardize decimal handling across the codebase, which could lead to improved consistency and potentially better performance. However, it's important to thoroughly test these changes to prevent any regressions in functionality or precision.
Consider adding comprehensive unit tests that cover various edge cases for decimal operations, especially focusing on the
swapFeecalculations. Additionally, it may be beneficial to document the rationale behind moving to a customDecimalimplementation for future reference.packages/proto-codecs/src/codegen/osmosis/gamm/v1beta1/gov.ts (1)
Line range hint
301-301: LGTM: Decimal usage is consistentThe usage of the
Decimalclass for thespreadFactorfield in thePoolRecordWithCFMMLinkinterface and its encoding/decoding functions is consistent and appears to be correct. However, for improved clarity, consider adding a comment explaining the significance of the '18' value used inDecimal.fromUserInputandDecimal.fromAtomics.Consider adding a comment like this:
// 18 represents the decimal places for the spread factorAlso applies to: 349-349
packages/proto-codecs/src/codegen/osmosis/superfluid/superfluid.ts (2)
Line range hint
496-586: LGTM. Consider adding test cases for decimal handling.The updates to the
OsmoEquivalentMultiplierRecordinterface, particularly in theencode,decode,fromAmino, andtoAminomethods, look good. These changes ensure proper handling of themultiplierfield as aDecimaltype, which should provide more precise and consistent decimal value management in the Osmosis protocol.To ensure the correct handling of decimal values, consider adding test cases that cover various scenarios, such as:
- Encoding and decoding a
multiplierwith different decimal places.- Converting between user input, atomic representation, and Amino format.
- Edge cases like very large or very small decimal values.
Here's a sample test case structure you could use:
describe('OsmoEquivalentMultiplierRecord', () => { it('should correctly encode and decode multiplier', () => { const record = { epochNumber: BigInt(1), denom: 'uosmo', multiplier: '1.5' }; const encoded = OsmoEquivalentMultiplierRecord.encode(record, BinaryWriter.create()).finish(); const decoded = OsmoEquivalentMultiplierRecord.decode(encoded); expect(decoded.multiplier).toBe(record.multiplier); }); it('should correctly convert to and from Amino', () => { const record = { epochNumber: BigInt(1), denom: 'uosmo', multiplier: '0.0001' }; const amino = OsmoEquivalentMultiplierRecord.toAmino(record); const fromAmino = OsmoEquivalentMultiplierRecord.fromAmino(amino); expect(fromAmino.multiplier).toBe(record.multiplier); }); });
Line range hint
1-586: Summary: Changes improve decimal handling in superfluid staking protocolThe modifications in this file, particularly the switch to a custom
Decimalimplementation and the updates to theOsmoEquivalentMultiplierRecordinterface, appear to enhance the precision and consistency of decimal value handling in the Osmosis superfluid staking protocol. These changes align well with the PR objectives of addressing transaction issues related to Decimal changes.As the project moves forward:
- Ensure that the custom
Decimalimplementation is thoroughly tested and documented.- Consider creating a migration guide if this change affects other parts of the codebase or external integrations.
- Monitor the performance impact of the new
Decimalimplementation, especially in high-throughput scenarios.packages/trpc/src/concentrated-liquidity.ts (1)
141-142: Ensure safe access toctx.chainList[0].To prevent potential runtime errors if
ctx.chainListis empty, consider adding a check to ensure thatctx.chainListhas at least one element before accessingctx.chainList[0].chain_id.Also applies to: 176-176
packages/web/components/complex/pool/create/cl/set-base-info.tsx (2)
Line range hint
149-167: Handle potential undefined values for 'selectedQuote' in the transaction functionIn the
onClickhandler for the 'Create Pool' button,selectedQuote?.token.coinMinimalDenom!may beundefinedifselectedQuoteis not selected, which could result in a runtime error. Ensure thatselectedQuoteis defined before accessing its properties or adjust the code to handle undefined values safely.Suggestion:
account?.osmosis .sendCreateConcentratedPoolMsg( selectedBase?.token.coinMinimalDenom!, - selectedQuote?.token.coinMinimalDenom!, + selectedQuote?.token.coinMinimalDenom!, 100, +selectedSpread, undefined, (res) => { /* ... */ } )Alternatively, ensure that both
selectedBaseandselectedQuoteare non-null before calling the function.
74-77: Use descriptive variable names in filter functions for better readabilityIn the filter functions for
baseTokensandquoteTokens, the variableqcis used to represent the token. Using a more descriptive name liketokenorassetcan improve code readability and maintainability.Suggestion:
assets={baseTokens.filter( - (qc) => - qc.token.coinDenom !== selectedQuote?.token.coinDenom + (token) => + token.token.coinDenom !== selectedQuote?.token.coinDenom )} assets={quoteTokens.filter( - (qc) => - qc.token.coinDenom !== selectedBase?.token.coinDenom + (token) => + token.token.coinDenom !== selectedBase?.token.coinDenom )}Also applies to: 91-94
packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/params.ts (3)
Line range hint
174-178: Fix unnecessary parentheses intoStringmethod callIn the
toAminofunction, thetoStringmethod is called with unnecessary parentheses, which could lead to readability issues or potential errors.Current code:
obj.hook_gas_limit = message.hookGasLimit !== BigInt(0) ? (message.hookGasLimit?.toString)() : undefined;The parentheses around
(message.hookGasLimit?.toString)are unnecessary. It would be clearer and more conventional to call the method directly.Apply this diff to correct the method call:
obj.hook_gas_limit = message.hookGasLimit !== BigInt(0) - ? (message.hookGasLimit?.toString)() + ? message.hookGasLimit?.toString() : undefined;
Line range hint
132-135: Handle potential null or undefined values infromAminoIn the
fromAminofunction, when assigninghookGasLimit, consider handling cases whereobject.hook_gas_limitmight be undefined or null to prevent runtime errors.Apply this diff to ensure safety:
if (object.hook_gas_limit !== undefined && object.hook_gas_limit !== null) { message.hookGasLimit = BigInt(object.hook_gas_limit); +} else { + message.hookGasLimit = BigInt(0); }
Line range hint
30-34: Update documentation forbalancer_shares_reward_discountThe documentation for
balancer_shares_reward_discountmentions that the field can range from(0,1]. Since you're now using theDecimalclass with 18 decimal places, consider updating the documentation to reflect how values should be represented (e.g., as strings with up to 18 decimal places).Clarify in the comments how the decimal precision affects the expected input format.
packages/stores/src/account/base.ts (1)
1074-1099: Remove debugging statementconsole.logThe
console.log(messages, normalizedMessages);statement at line 1097 appears to be for debugging purposes. It's advisable to remove it to prevent unnecessary logging in production code.Apply this diff to remove the debug statement:
- console.log(messages, normalizedMessages);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (21)
.github/workflows/jest-tests.ymlis excluded by!**/*.ymlpackages/proto-codecs/package.jsonis excluded by!**/*.jsonpackages/proto-codecs/tsconfig.jsonis excluded by!**/*.jsonpackages/web/localizations/de.jsonis excluded by!**/*.jsonpackages/web/localizations/en.jsonis excluded by!**/*.jsonpackages/web/localizations/es.jsonis excluded by!**/*.jsonpackages/web/localizations/fa.jsonis excluded by!**/*.jsonpackages/web/localizations/fr.jsonis excluded by!**/*.jsonpackages/web/localizations/gu.jsonis excluded by!**/*.jsonpackages/web/localizations/hi.jsonis excluded by!**/*.jsonpackages/web/localizations/ja.jsonis excluded by!**/*.jsonpackages/web/localizations/ko.jsonis excluded by!**/*.jsonpackages/web/localizations/pl.jsonis excluded by!**/*.jsonpackages/web/localizations/pt-br.jsonis excluded by!**/*.jsonpackages/web/localizations/ro.jsonis excluded by!**/*.jsonpackages/web/localizations/ru.jsonis excluded by!**/*.jsonpackages/web/localizations/tr.jsonis excluded by!**/*.jsonpackages/web/localizations/zh-cn.jsonis excluded by!**/*.jsonpackages/web/localizations/zh-hk.jsonis excluded by!**/*.jsonpackages/web/localizations/zh-tw.jsonis excluded by!**/*.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (42)
- packages/proto-codecs/jest.config.js (1 hunks)
- packages/proto-codecs/scripts/tests/decimal-patch.spec.ts (1 hunks)
- packages/proto-codecs/scripts/codegen.ts (3 hunks)
- packages/proto-codecs/scripts/decimals-patch.ts (1 hunks)
- packages/proto-codecs/src/codegen/binary.ts (1 hunks)
- packages/proto-codecs/src/codegen/cosmos/staking/v1beta1/staking.ts (1 hunks)
- packages/proto-codecs/src/codegen/cosmos/staking/v1beta1/tx.ts (1 hunks)
- packages/proto-codecs/src/codegen/decimals.ts (1 hunks)
- packages/proto-codecs/src/codegen/helpers.ts (1 hunks)
- packages/proto-codecs/src/codegen/index.ts (2 hunks)
- packages/proto-codecs/src/codegen/osmosis/accum/v1beta1/accum.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/params.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/poolmodel/concentrated/v1beta1/tx.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/v1beta1/gov.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/v1beta1/incentive_record.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/v1beta1/pool.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/v1beta1/position.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/v1beta1/tick_info.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/v1beta1/tx.ts (2 hunks)
- packages/proto-codecs/src/codegen/osmosis/cosmwasmpool/v1beta1/model/module_query_msg.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/cosmwasmpool/v1beta1/model/module_sudo_msg.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/cosmwasmpool/v1beta1/model/pool_query_msg.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/gamm/poolmodels/stableswap/v1beta1/stableswap_pool.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/gamm/v1beta1/balancerPool.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/gamm/v1beta1/gov.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/poolmanager/v1beta1/genesis.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/poolmanager/v1beta1/taker_fee_share.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/poolmanager/v1beta1/tx.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/superfluid/params.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/superfluid/superfluid.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/superfluid/tx.ts (1 hunks)
- packages/proto-codecs/src/codegen/osmosis/valsetpref/v1beta1/state.ts (1 hunks)
- packages/proto-codecs/src/codegen/utf8.ts (1 hunks)
- packages/proto-codecs/src/codegen/varint.ts (1 hunks)
- packages/server/src/queries/osmosis/concentratedliquidity/index.ts (1 hunks)
- packages/server/src/queries/osmosis/concentratedliquidity/params.ts (1 hunks)
- packages/server/src/queries/osmosis/poolmanager/params.ts (1 hunks)
- packages/stores/src/account/amino-converters.ts (2 hunks)
- packages/stores/src/account/base.ts (4 hunks)
- packages/trpc/src/concentrated-liquidity.ts (2 hunks)
- packages/web/components/complex/pool/create/cl-pool.tsx (4 hunks)
- packages/web/components/complex/pool/create/cl/set-base-info.tsx (7 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/proto-codecs/src/codegen/binary.ts
- packages/proto-codecs/src/codegen/helpers.ts
- packages/proto-codecs/src/codegen/utf8.ts
- packages/proto-codecs/src/codegen/varint.ts
🧰 Additional context used
🔇 Additional comments (91)
packages/server/src/queries/osmosis/concentratedliquidity/index.ts (1)
4-4: LGTM! New export added consistently.The addition of
export * from "./params";is consistent with the existing export statements and enhances the module's interface by including the contents of theparamsmodule.To ensure the correctness of this change, please run the following verification script:
This script will verify the existence of the
params.tsfile, check if it contains exports, and search for any imports of theparamsmodule in the project.packages/server/src/queries/osmosis/concentratedliquidity/params.ts (2)
1-1: LGTM: Import statement is correct.The import statement for
createNodeQueryis properly structured and follows relative path conventions.
1-18: Summary: Implementation aligns well with PR objectives.This new file successfully implements the necessary types and query for concentrated liquidity parameters. It supports the PR objectives by providing a structured way to retrieve parameters essential for various pool operations, including creating pools and handling liquidity positions.
The implementation is correct, comprehensive, and follows good TypeScript practices. The minor suggestions provided earlier will further enhance readability and consistency.
packages/proto-codecs/jest.config.js (6)
6-8: moduleNameMapper configuration is appropriate for ES modules.The moduleNameMapper configuration correctly handles .js extensions in import statements. This is crucial when working with ECMAScript modules in Node.js, ensuring compatibility between TypeScript and Node.js module resolution.
14-20: TypeScript transform configuration is well-configured.The transform configuration for TypeScript files using ts-jest with ECMAScript modules support (useESM: true) is appropriate. This setup aligns well with modern JavaScript practices and ensures proper handling of TypeScript files in the Jest environment.
21-24: Watch plugins configuration enhances developer experience.The inclusion of jest-watch-typeahead plugins for filename and testname is a great addition. These plugins significantly improve the development experience by allowing interactive filtering of test files and test names during watch mode, enhancing developer productivity.
1-25: Overall Jest configuration is well-structured and comprehensive.The Jest configuration file is well-crafted, covering all necessary aspects for testing a TypeScript project with support for ECMAScript modules. It includes appropriate presets, transformers, and developer experience enhancements. The configuration aligns well with modern JavaScript practices and should provide a robust testing environment for the proto-codecs package.
9-13: JavaScript transform configuration is correct, but note the external Babel config.The transform configuration for JavaScript files using babel-jest is appropriate. However, the Babel configuration file is located outside the current package directory (../../babel.config.json).
Ensure that this external Babel configuration is intentional and accessible. You can verify its existence and content with:
#!/bin/bash # Description: Verify the existence and content of the Babel configuration file # Test: Check if the Babel config file exists and display its content if [ -f ../../babel.config.json ]; then echo "Babel configuration file exists. Content:" cat ../../babel.config.json else echo "Babel configuration file not found at ../../babel.config.json" fi
1-5: Basic Jest configuration looks good, but consider the long timeout.The basic Jest configuration is well-structured for a TypeScript project. However, the test timeout of 100 seconds (100000 ms) is quite long. This might indicate complex or time-consuming tests.
Consider reviewing your tests to see if they can be optimized. If the long timeout is necessary, it would be helpful to add a comment explaining why. You can run the following command to identify slow tests:
packages/proto-codecs/src/codegen/index.ts (2)
13-13: New export added for the 'decimals' moduleA new export statement
export * from "./decimals";has been added. This aligns with the PR objectives to address Decimal-related issues.Let's verify the existence and content of the new 'decimals' module:
✅ Verification successful
The 'decimals' module exists and its content aligns with the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the decimals.ts file exists and show its content cat packages/proto-codecs/src/codegen/decimals.tsLength of output: 3235
3-3: Version update in the comment headerThe version of
@cosmology/telescopehas been updated from 1.8.4 to 1.10.0. This change reflects an update in the tool used to generate this file.To ensure consistency, let's verify if this version update is reflected in the project's dependencies:
packages/server/src/queries/osmosis/poolmanager/params.ts (1)
14-18: LGTM! The changes align well with the PR objectives.The additions to the
PoolmanagerParamsResponsetype are well-structured and relevant:
authorized_quote_denoms: string[]likely represents a list of allowed quote denominations for pools.pool_creation_fee: { denom: string; amount: string; }[]defines the fee structure for creating new pools.These changes support the PR objective of addressing issues related to "creating all types of pools" and align with the overall goal of fixing transaction issues arising from Decimal changes in Osmosis version 26.
packages/proto-codecs/scripts/__tests__/decimal-patch.spec.ts (2)
10-14: Well-structured test for Decimal creation from user input.This test case effectively validates the creation of a Decimal instance from user input, checking both the atomic value and the number of fractional digits. It covers the basic functionality of the
fromUserInputmethod.
1-54: Excellent test coverage for Decimal implementations.This comprehensive test suite for the Decimal class implementations is crucial for ensuring the correctness of decimal handling in the context of the PR objectives. It effectively covers creation from user input and atomics, error handling, edge cases, and precision handling.
The thorough testing of both
CodegenDecimalandDecimalPatchimplementations ensures consistency and reliability in decimal operations, which is essential for accurate transaction processing in the Osmosis platform. This aligns well with the PR's goal of addressing transaction issues related to Decimal changes in Osmosis version 26.The test suite provides a solid foundation for verifying the correct encoding of messages into the proto format, particularly for standardizing Decimal data types. This contributes to the PR's objectives of ensuring correct processing of various transaction types, including unbonding weighted pool shares, withdrawing concentrated liquidity positions, and creating all types of pools.
packages/proto-codecs/scripts/codegen.ts (3)
2-2: LGTM: Proper import of thefsmodule.The import of the
fsmodule using thenode:prefix is correct and follows best practices for importing built-in Node.js modules.
Line range hint
1-126: Summary: Changes align with PR objectives, final verifications needed.The modifications to this file, including the addition of the
usePatchedDecimaloption and the implementation of the patching process, align well with the PR objectives of addressing Decimal changes in Osmosis version 26. These changes should contribute to correctly encoding messages into the proto format, particularly for transactions involving Decimal values.To ensure the effectiveness of these changes:
- Verify that the generated code uses the patched Decimal implementation correctly.
- Test the following functionalities as mentioned in the PR objectives:
- Unbonding weighted pool shares
- Withdrawing concentrated liquidity positions
- Creating all types of pools
- Swaps
Run the following script to check if the patched Decimal is being used in the generated code and if it's applied to the relevant transaction types:
#!/bin/bash # Description: Verify usage of patched Decimal in generated code and relevant transactions # Test 1: Check for Decimal usage in generated files echo "Checking Decimal usage in generated files:" rg --type typescript 'import.*Decimal' src/codegen # Test 2: Check for usage in relevant transaction types echo "\nChecking usage in relevant transaction types:" rg --type typescript -i '(unbond|withdraw|create.*pool|swap).*decimal' src/codegenIf these checks pass and the manual testing of the mentioned functionalities is successful, we can be confident that the changes have addressed the Decimal issues as intended.
90-90: Verify the impact of using the patched Decimal.The addition of
usePatchedDecimal: truealigns with the PR objective of addressing Decimal changes. This should help in correctly encoding messages into the proto format.To ensure this change has the desired effect, please verify that:
- The generated code uses the patched Decimal implementation.
- Transactions involving Decimals (e.g., unbonding weighted pool shares, withdrawing concentrated liquidity positions) work as expected.
You can use the following script to check for the usage of the patched Decimal in the generated code:
packages/proto-codecs/src/codegen/osmosis/superfluid/params.ts (3)
Line range hint
63-67: Ensure consistency in precision between encoding and decodingThe
decodemethod usesDecimal.fromAtomicswith a precision of 18 decimals, which matches the precision used in theencodemethod. This consistency is good, but it's important to verify that it maintains the same behavior as the previous implementation.Please ensure that:
- The precision of 18 decimals in
Decimal.fromAtomicsis consistent with the previous decoding implementation.- The
toString()method of the newDecimalclass produces output identical to the previous implementation.Consider adding a unit test that:
- Encodes a known
Paramsobject- Decodes the resulting binary data
- Compares the decoded object with the original to ensure perfect round-trip conversion
Would you like help in creating this unit test to verify the consistency of encoding and decoding?
Line range hint
47-51: Verify precision inDecimal.fromUserInputThe
encodemethod usesDecimal.fromUserInputwith a precision of 18 decimals. While this is likely correct, it's crucial to ensure that this precision is consistent with the previous implementation and meets the requirements of the superfluid module.Please confirm that:
- The precision of 18 decimals is correct for the
minimumRiskFactorin the context of the superfluid module.- This precision is consistent with how
minimumRiskFactoris used in other parts of the codebase.Run the following script to check for other usages of
minimumRiskFactorand verify the consistency of precision:#!/bin/bash # Search for other usages of minimumRiskFactor rg --type typescript 'minimumRiskFactor' -C 3
3-3: Verify compatibility of the newDecimalimplementationThe import for the
Decimalclass has been changed from an external library (@cosmjs/math) to a local implementation (../../decimals). This change could potentially affect the behavior of decimal operations in this file.Please ensure that:
- The new
Decimalimplementation is fully compatible with the previous one, especially for thefromUserInput,fromAtomics, andtoStringmethods used in this file.- The precision and rounding behavior remain consistent with the previous implementation.
- Comprehensive tests are in place to verify that this change doesn't introduce any regressions in the handling of
minimumRiskFactor.Run the following script to check for any other usages of the
Decimalclass that might need updating:Would you like assistance in creating additional unit tests to verify the compatibility of the new
Decimalimplementation?✅ Verification successful
Decimal import change verified
The
Decimalclass import has been successfully updated without affecting other parts of the codebase. No additional usages of@cosmjs/mathwere found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of Decimal from @cosmjs/math rg --type typescript 'from "@cosmjs/math"' -C 3Length of output: 174
Script:
#!/bin/bash # Search for other usages of Decimal from @cosmjs/math across TypeScript files rg 'from "@cosmjs/math"' --glob '*.ts' -C 3Length of output: 45
packages/web/components/complex/pool/create/cl-pool.tsx (3)
9-9: LGTM: Import for internationalization added.The addition of
MultiLanguageTanduseTranslationimports is appropriate for implementing internationalization in the component.
48-48: LGTM: Internationalization implemented in CreateCLPool component.The changes correctly implement internationalization:
- The
useTranslationhook is used to obtain the translation function.- The
getStepHeaderfunction is now called with the translation function as an argument.These modifications ensure that the component can render text in multiple languages.
Also applies to: 101-102
123-127: LGTM: getStepHeader function signature updated for internationalization.The function signature has been correctly updated to include the translation function as its first parameter. This change:
- Allows the function to use translations.
- Maintains type safety with the
MultiLanguageTtype.- Preserves the existing parameters, ensuring backwards compatibility.
packages/stores/src/account/amino-converters.ts (2)
Line range hint
66-134: Consider aligningfromAminowithtoAminofor consistency.The
toAminomethod has been updated to remove thetimeout_timestampfield, which aligns with the comment stating it's not used by your transactions. However, thefromAminomethod still includestimeout_timestampin its parameters and return value. This inconsistency might lead to potential issues.Consider updating the
fromAminomethod to match thetoAminomethod by removing thetimeout_timestamphandling:fromAmino: ({ source_port, source_channel, token, sender, receiver, timeout_height, - timeout_timestamp, }: AminoMsgTransfer["value"]): MsgTransfer => { return { sourcePort: source_port, sourceChannel: source_channel, token: { denom: token?.denom ?? "", amount: token?.amount ?? "", }, sender, receiver, timeoutHeight: timeout_height ? { revisionHeight: Long.fromString( timeout_height.revision_height || "0", true ), revisionNumber: Long.fromString( timeout_height.revision_number || "0", true ), } : undefined, - timeoutTimestamp: Long.fromString(timeout_timestamp ?? "0"), }; },Let's verify the impact of this change on IBC transfers:
#!/bin/bash # Description: Check for usage of MsgTransfer and timeoutTimestamp in the codebase # Test: Search for MsgTransfer usage rg --type typescript -i 'MsgTransfer' # Test: Search for timeoutTimestamp usage rg --type typescript -i 'timeoutTimestamp'
59-65: LGTM! Verify usage of the new converter.The addition of the
MsgCreateConcentratedPoolconverter with the explicitaminoTypeis a good improvement. This change aligns with the PR objective of enabling the creation of all types of pools.Let's verify the usage of this new converter:
✅ Verification successful
Verified usage of the new converter.
The
MsgCreateConcentratedPoolconverter with theaminoType"osmosis/cl-create-pool" is correctly integrated and used appropriately within the codebase. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of MsgCreateConcentratedPool in the codebase # Test: Search for MsgCreateConcentratedPool usage rg --type typescript -i 'MsgCreateConcentratedPool' # Test: Search for cl-create-pool amino type usage rg --type typescript -i 'cl-create-pool'Length of output: 253
Script:
#!/bin/bash # Description: Check for usage of MsgCreateConcentratedPool in the codebase # Test: Search for MsgCreateConcentratedPool usage in all files rg -i 'MsgCreateConcentratedPool' # Test: Search for cl-create-pool amino type usage in all files rg -i 'cl-create-pool'Length of output: 15026
packages/proto-codecs/src/codegen/osmosis/valsetpref/v1beta1/state.ts (1)
3-3: Verify compatibility of the new Decimal implementationThe import for
Decimalhas been changed from an external library (@cosmjs/math) to a local implementation (../../../decimals). This change aligns with the PR objective of addressing Decimal changes in Osmosis v26.Please ensure that:
- The new local Decimal implementation provides all the necessary functionality used in this file.
- It maintains compatibility with the previous implementation to avoid any breaking changes.
- The new implementation correctly handles the precision and range required for validator preferences.
To verify the compatibility, run the following script:
This script will help confirm that the new Decimal implementation includes the
fromUserInputandfromAtomicsmethods used in this file.✅ Verification successful
Decimal implementation is compatible and functional
The new
Decimalimplementation inpackages/proto-codecs/src/codegen/decimals.tsincludes the requiredfromUserInputandfromAtomicsmethods. This ensures compatibility with the previous external library (@cosmjs/math) and aligns with the PR objective of addressing Decimal changes in Osmosis v26.
- Verified the presence of
fromUserInputandfromAtomicsmethods in the local Decimal implementation.- Confirmed that these methods are available for handling precision and range as needed for validator preferences.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the new Decimal implementation has the required methods grep -R "fromUserInput" packages/proto-codecs/src/decimals grep -R "fromAtomics" packages/proto-codecs/src/decimalsLength of output: 251
Script:
#!/bin/bash # Search for the Decimal class implementation in the codebase fd -e ts "class Decimal" .Length of output: 28
Script:
#!/bin/bash # Search for any occurrence of 'Decimal' in the codebase rg "Decimal"Length of output: 131002
Script:
#!/bin/bash # Check if the new Decimal implementation has the required methods grep -R "fromUserInput" packages/proto-codecs/src/codegen/decimals.ts grep -R "fromAtomics" packages/proto-codecs/src/codegen/decimals.tsLength of output: 197
packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/poolmodel/concentrated/v1beta1/tx.ts (2)
Line range hint
1-359: Summary: Improved decimal handling for concentrated liquidity poolsThe changes in this file significantly improve the handling of decimal values, particularly for the
spreadFactorinMsgCreateConcentratedPool. By using a localDecimalimplementation and standardizing the encoding and decoding processes, these modifications directly address the transaction issues mentioned in the PR objectives.These improvements should resolve the problems with creating concentrated liquidity pools, which was one of the key issues highlighted in the PR summary. The consistent use of 18 decimal places precision ensures accurate representation of decimal values throughout the transaction lifecycle.
To fully validate these changes:
- Ensure that all other message types that use decimal values are updated similarly.
- Verify that these changes resolve the issues with creating all types of pools, as mentioned in the PR objectives.
- Test the full transaction flow, including encoding, transmission, and decoding, to confirm that decimal values are handled correctly end-to-end.
To verify the impact of these changes across the codebase, run the following script:
#!/bin/bash # Description: Check for consistent use of the new Decimal implementation # Test: Search for other uses of Decimal in message encoding/decoding rg --type typescript -g 'src/codegen/**/*.ts' 'Decimal\.from(UserInput|Atomics)' # Test: Check for any remaining uses of the old @cosmjs/math Decimal rg --type typescript '@cosmjs/math'This will help ensure that the Decimal changes have been applied consistently across all relevant parts of the codebase.
3-3: LGTM. Verify local Decimal implementation.The change from using
@cosmjs/mathto a localDecimalimplementation aligns with the PR objectives. This should help address the transaction issues related to Decimal changes.To ensure compatibility, please verify that the local
Decimalimplementation in../../../../../decimalsprovides the same functionality as the previous@cosmjs/mathimplementation. Run the following script to check theDecimalclass implementation:packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/v1beta1/pool.ts (4)
Line range hint
195-198: Approve decimal conversion changes in decode method.The changes in the
decodemethod forcurrentTickLiquidityandspreadFactorare approved. UsingDecimal.fromAtomicsensures proper conversion from atomic representation to string representation during decoding.Please verify that the decoding process is consistent with the encoding process and other parts of the system. Run the following script to check for any inconsistencies in decimal handling:
#!/bin/bash # Description: Check for consistency in decimal handling # Test: Search for other usages of Decimal.fromAtomics and Decimal.fromUserInput rg --type typescript -e "Decimal\.fromAtomics|Decimal\.fromUserInput"Also applies to: 218-221
Line range hint
307-310: Approve Amino conversion changes and verify consistency.The changes in the
toAminomethod forid,currentTick,tickSpacing, andexponentAtPriceOneare approved. Converting these fields to strings only when they have non-default values ensures a more efficient Amino representation.Please ensure that this change in Amino conversion is consistent with the system's expectations and doesn't break any existing functionality. Run the following script to check for other Amino conversions that might need similar updates:
#!/bin/bash # Description: Check for consistency in Amino conversions # Test: Search for other toAmino methods that might need similar updates rg --type typescript -e "toAmino.*BigInt\(0\)" -g '!pool.ts'Also applies to: 315-322
Line range hint
142-145: Approve decimal conversion changes and verify precision requirements.The changes in the
encodemethod forcurrentTickLiquidityandspreadFactorare approved. UsingDecimal.fromUserInputensures proper conversion to atomic representation before encoding.Please confirm that 18 decimal places of precision are sufficient for
currentTickLiquidityandspreadFactor. Run the following script to check for any other usages of these fields that might require adjustment:Also applies to: 165-168
3-3: Approve import change and verify local Decimal implementation.The change from importing
Decimalfrom@cosmjs/mathto a local implementation is approved. This shift could provide more control over theDecimalfunctionality and reduce external dependencies.Please ensure that the local
Decimalimplementation in../../../decimalsis thoroughly tested and provides all the necessary functionality previously offered by@cosmjs/math. Run the following script to verify the implementation:packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/v1beta1/tick_info.ts (3)
Line range hint
93-106: LGTM: TickInfo encoding and decoding logicThe changes in the
TickInfo.encodeandTickInfo.decodemethods appear to be correct and consistent with the newDecimalimport. The methods maintain the use of 18 decimal places for precision, which is crucial for compatibility. The encoding usesDecimal.fromUserInput, while decoding usesDecimal.fromAtomics, which is the expected pattern for these operations.Also applies to: 124-137
8-8: Verify compatibility of the new Decimal implementationThe import for the
Decimalclass has been changed from@cosmjs/mathto a custom implementation. This change could potentially affect the behavior of Decimal operations in theTickInfo.encodeandTickInfo.decodemethods.Please ensure that the new
Decimalimplementation in../../../decimalsis fully compatible with the previous one from@cosmjs/math, especially for thefromUserInput,fromAtomics, andatomicsmethods used in this file. Run the following script to compare the implementations:#!/bin/bash # Description: Compare the new Decimal implementation with @cosmjs/math # Check if the new Decimal file exists if [ ! -f "packages/proto-codecs/src/decimals.ts" ]; then echo "Error: New Decimal implementation file not found" exit 1 fi # Compare the methods used in this file echo "Comparing Decimal implementations:" echo "1. fromUserInput method:" grep -n "fromUserInput" packages/proto-codecs/src/decimals.ts echo "2. fromAtomics method:" grep -n "fromAtomics" packages/proto-codecs/src/decimals.ts echo "3. atomics property:" grep -n "atomics" packages/proto-codecs/src/decimals.ts # Check for any usage of @cosmjs/math in the project echo "Checking for any remaining usage of @cosmjs/math:" grep -R "@cosmjs/math" packages/This script will help verify that the new
Decimalimplementation includes all necessary methods and properties, and that there are no remaining references to@cosmjs/mathin the project.
Line range hint
1-430: Summary: Minor change with potential for wider impactThe main change in this file is the import of
Decimalfrom a custom implementation instead of@cosmjs/math. While the usage ofDecimalin theTickInfomethods remains consistent, this change could have broader implications:
- It's part of a larger refactoring to use a custom
Decimalimplementation across the project.- The change doesn't affect the overall logic of this file, but it's crucial to ensure that the new
Decimalimplementation is fully compatible with the previous one.- The precision and behavior of decimal operations must be maintained to avoid any subtle bugs in financial calculations.
To ensure the change doesn't introduce any regressions, please:
- Verify that all tests related to
TickInfoand other components usingDecimalpass with the new implementation.- Consider adding integration tests that specifically check the encoding and decoding of
TickInfowith various decimal values.- Document the reason for moving to a custom
Decimalimplementation and any differences from@cosmjs/mathfor future reference.This script will help ensure that the changes haven't introduced any regressions in the
TickInfofunctionality or other areas using theDecimalclass.packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/v1beta1/incentive_record.ts (1)
Line range hint
1-385: Summary: Changes successfully address Decimal-related transaction issues.The modifications in this file effectively implement the necessary changes to handle decimal values consistently, particularly for the
emissionRateinIncentiveRecordBody. The switch to an internalDecimalimplementation and the proper conversion between user input, atomic representation, and string output align well with the PR objectives. These changes should contribute to resolving the transaction issues that arose due to Decimal changes in Osmosis version 26.packages/proto-codecs/src/codegen/osmosis/gamm/poolmodels/stableswap/v1beta1/stableswap_pool.ts (2)
Line range hint
1-462: Overall LGTM! Suggest comprehensive testing.The changes in this file effectively address the Decimal-related issues in transaction processing, aligning well with the PR objectives. The modifications to the
PoolParamsencoding and decoding methods using the newDecimalimplementation are consistent and well-implemented.To ensure the robustness of these changes, consider implementing comprehensive test cases that cover various scenarios, including:
- Encoding and decoding of
PoolParamswith different fee values (including edge cases).- Integration tests that simulate the full lifecycle of transactions involving these Decimal conversions.
- Compatibility tests with existing data to ensure backward compatibility.
Would you like assistance in designing these test cases?
8-8: LGTM! Verify the new Decimal implementation.The change from importing
Decimalfrom "@cosmjs/math" to a local implementation "../../../../../decimals" aligns with the PR objective of addressing transaction issues related to Decimal changes. This shift allows for more control over the Decimal implementation.To ensure the new
Decimalimplementation is consistent with the project's requirements, please run the following verification script:✅ Verification successful
Verified the Decimal implementation path.
The
decimals.tsfile exists atpackages/proto-codecs/src/codegen/decimals.ts, and the import statement instableswap_pool.tscorrectly references it using"../../../../../decimals". The newDecimalimplementation is properly integrated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new Decimal implementation and its usage # Test: Check if the new Decimal implementation exists if [ ! -f "packages/proto-codecs/src/decimals.ts" ]; then echo "Error: New Decimal implementation file not found" exit 1 fi # Test: Verify Decimal usage in the project rg --type typescript 'import.*Decimal.*from.*decimals' packages/proto-codecs/srcLength of output: 170
Script:
#!/bin/bash # Description: Search the entire repository for the decimals.ts file fd decimals.ts . --type fLength of output: 75
packages/proto-codecs/src/codegen/osmosis/accum/v1beta1/accum.ts (2)
Line range hint
1-458: Summary: Changes align well with PR objectivesThe modifications in this file consistently address the Decimal-related transaction issues mentioned in the PR objectives. The changes to
AccumulatorContentandRecordencoding methods ensure that Decimal values are properly converted to their atomic representations before being written.Key points:
- The
Decimalimport has been changed to use an internal implementation.- Both
totalSharesandnumSharesare now encoded usingDecimal.fromUserInput(...).atomics.- These changes should help standardize the handling of Decimal types in transactions.
The suggestions provided for making decimal places configurable could further enhance the flexibility of the implementation. Overall, these changes appear to be a solid step towards resolving the issues outlined in the PR objectives.
8-8: Verify the rationale for changing the Decimal import.The import for
Decimalhas been changed from an external library (@cosmjs/math) to an internal implementation (../../../decimals). While this change might align with the PR objectives to address transaction issues related to Decimal changes, it's important to verify:
- The compatibility of the new
Decimalimplementation with the existing codebase.- The potential impact on other parts of the system that might be using the
@cosmjs/mathDecimal.- The benefits of using an internal implementation over the external library.
To ensure this change doesn't introduce unintended side effects, please run the following script:
This will help identify any inconsistencies in Decimal usage across the codebase.
✅ Verification successful
Decimal import change verified
All instances of
@cosmjs/math Decimalhave been successfully replaced with the internal../../../decimalsimplementation. No remaining references to the external library were found across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other usages of @cosmjs/math Decimal rg --type typescript '@cosmjs/math.*Decimal' # Check for other files using the new Decimal implementation rg --type typescript 'from "../../../decimals"'Length of output: 166
Script:
#!/bin/bash # Check for other usages of @cosmjs/math Decimal rg --type ts '@cosmjs/math.*Decimal' # Check for other files using the new Decimal implementation rg --type ts 'from "../../../decimals"'Length of output: 1874
Script:
#!/bin/bash # Check for other usages of @cosmjs/math Decimal rg --type ts '@cosmjs/math.*Decimal' # Check for other files using the new Decimal implementation rg --type ts 'from "../../../decimals"'Length of output: 1874
packages/proto-codecs/src/codegen/osmosis/poolmanager/v1beta1/taker_fee_share.ts (1)
8-8: Verify the impact of Decimal import change across the codebaseThe import for
Decimalhas been changed from an external library to a local implementation. This change may have implications for decimal handling throughout the codebase.To ensure consistency and compatibility, run the following script to check for any remaining imports of Decimal from "@cosmjs/math" and usages of the new local Decimal implementation:
✅ Verification successful
Decimal import change verified successfully
All
Decimalimports have been updated from@cosmjs/mathto the local implementation../../../decimals. No remaining imports from the old library were found, and all usages of the newDecimalmethods are consistent across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for remaining imports from @cosmjs/math echo "Checking for remaining @cosmjs/math Decimal imports:" rg --type typescript 'import.*Decimal.*from.*@cosmjs/math' # Check for usages of the new local Decimal implementation echo "Checking for usages of the new local Decimal implementation:" rg --type typescript 'import.*Decimal.*from.*"../../../decimals"' rg --type typescript 'Decimal\.fromUserInput|Decimal\.fromAtomics'Length of output: 548
Script:
#!/bin/bash # Check for remaining imports from @cosmjs/math echo "Checking for remaining @cosmjs/math Decimal imports in .ts and .tsx files:" rg 'import.*Decimal.*from.*@cosmjs/math' --glob '**/*.ts' --glob '**/*.tsx' # Check for usages of the new local Decimal implementation echo "Checking for usages of the new local Decimal implementation in .ts and .tsx files:" rg 'import.*Decimal.*from.*"../../../decimals"' --glob '**/*.ts' --glob '**/*.tsx' rg 'Decimal\.fromUserInput|Decimal\.fromAtomics' --glob '**/*.ts' --glob '**/*.tsx'Length of output: 15959
packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/v1beta1/position.ts (2)
Line range hint
1-638: Summary of changes and their impactThe changes in this file improve the handling of decimal values for liquidity in concentrated liquidity positions. The main improvements are:
- Switching to a local
Decimalimplementation.- Consistent encoding and decoding of liquidity values using atomic representation.
These changes enhance the reliability and consistency of decimal operations throughout the system. The suggested minor improvements (extracting magic numbers to constants) would further increase code readability and maintainability.
8-8: Verify the impact of the new Decimal importThe import for
Decimalhas been changed from an external library to a local implementation. This change could potentially affect the behavior of decimal operations throughout the file.To ensure consistency, run the following script to check for any other usages of the old import:
✅ Verification successful
Adjust the search command to properly recognize TypeScript files
The previous script failed because "typescript" is not a recognized file type in ripgrep. Updating the file type to "ts" should correctly target TypeScript files.
Import change verified successfully
All imports from
@cosmjs/mathhave been removed and replaced with the local../../../decimalsimplementation. No remaining instances of@cosmjs/mathwere found in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining imports from @cosmjs/math rg --type typescript 'from "@cosmjs/math"'Length of output: 79
Script:
#!/bin/bash # Search for any remaining imports from @cosmjs/math rg --type ts 'from "@cosmjs/math"'Length of output: 36
packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/v1beta1/gov.ts (2)
Line range hint
1-453: Summary of changes and recommendationsThe main changes in this file revolve around the implementation and usage of the
Decimaltype, particularly for thespreadFactorfield in thePoolRecordinterface. These modifications aim to improve precision and compatibility in handling decimal values within the concentrated liquidity pools functionality.Key points to address:
- Ensure the new local
Decimalimplementation is fully compatible with the previous external library.- Verify that the encoding and decoding processes for
PoolRecord, especially for thespreadFactorfield, maintain consistency and precision.- Consider adding or updating unit tests to cover these changes, particularly focusing on edge cases and various
spreadFactorvalues.These changes appear to be part of a larger effort to standardize and improve decimal handling within the Osmosis protocol. While the modifications seem reasonable, thorough testing and verification are crucial to maintain the integrity and reliability of the system.
Line range hint
392-393: Verify encoding and decoding of spreadFactor with new Decimal implementationThe
encodeanddecodemethods forPoolRecordhave been updated to use the newDecimalimplementation for handling thespreadFactor. While these changes appear to maintain precision and compatibility, it's crucial to verify that they don't introduce any breaking changes in the encoding/decoding process.Please ensure that:
- The encoded values are consistent with the previous implementation.
- Decoding of existing data works correctly with the new implementation.
- The precision of 18 decimal places is maintained throughout the process.
To confirm the correctness of these changes, please run the following script:
#!/bin/bash # Description: Verify encoding and decoding of spreadFactor # Find all files that use PoolRecord files=$(rg -l "PoolRecord" --type ts) # Check encoding and decoding usage for file in $files; do echo "Checking file: $file" rg "PoolRecord\.encode" "$file" -C 2 rg "PoolRecord\.decode" "$file" -C 2 done # Check if there are any existing tests for PoolRecord rg "describe\(.*PoolRecord" --type tsIf no existing tests are found, consider adding unit tests to verify the encoding and decoding process with various
spreadFactorvalues.Also applies to: 410-413
packages/proto-codecs/src/codegen/osmosis/gamm/v1beta1/balancerPool.ts (1)
8-8: Verify the impact of the Decimal import change.The import for
Decimalhas been changed from an external library to an internal implementation. This change could have implications across the codebase.Please run the following script to check for any inconsistencies in
Decimalusage:✅ Verification successful
Decimal import path successfully updated across the codebase.
All instances now import
Decimalfrom"../../../decimals", with no remaining imports from@cosmjs/math.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining imports from @cosmjs/math rg --type typescript 'from "@cosmjs/math"' # Check for usage of the new Decimal import rg --type typescript 'from "../../../decimals"'Length of output: 164
Script:
#!/bin/bash # Check for any remaining imports from @cosmjs/math in TypeScript files rg --glob '**/*.ts' 'from "@cosmjs/math"' # Check for usage of the new Decimal import in TypeScript files rg --glob '**/*.ts' 'from "../../../decimals"'Length of output: 1886
packages/proto-codecs/src/codegen/osmosis/cosmwasmpool/v1beta1/model/module_sudo_msg.ts (3)
Line range hint
522-525: Approve consistent implementation.The usage of the new
Decimalimplementation for convertingswapFeeto atomics inSwapExactAmountOut.encodeis consistent with the changes made inSwapExactAmountIn.encode. This ensures uniform handling of swap fees across different swap types.
Line range hint
225-228: Approve change and suggest precision test.The usage of the new
Decimalimplementation for convertingswapFeeto atomics is correct. This change ensures proper encoding of the swap fee.To verify that this change doesn't introduce any precision issues, consider adding a unit test that compares the results of the old and new implementations:
8-8: Verify compatibility of the new Decimal implementation.The import for
Decimalhas been changed from@cosmjs/mathto a local implementation. Ensure that this newDecimalclass is fully compatible with the rest of the codebase and provides the same functionality as the previous implementation.To verify the compatibility, run the following script:
packages/proto-codecs/src/codegen/osmosis/gamm/v1beta1/gov.ts (2)
Line range hint
1-1024: LGTM: Well-structured and consistent implementationThe overall structure and content of the file are well-organized and consistent. The implementation of various governance proposal interfaces and their corresponding encoding/decoding functions appears robust and error-free. This implementation aligns well with the PR objectives of addressing transaction issues related to Decimal changes.
3-3: Verify compatibility of the new Decimal implementationThe import for the
Decimalclass has been changed from an external library to a local implementation. While this change can provide more control over the Decimal functionality, it's crucial to ensure that the new implementation in../../../decimalsis fully compatible with the previous one from@cosmjs/math.To confirm compatibility, please run the following script:
This will help identify any potential inconsistencies in how Decimal is used across the codebase.
✅ Verification successful
Decimal import is consistent across the codebase
The
Decimalclass is uniformly imported from../../../decimalsin all relevant files, ensuring a seamless transition from the external library to the local implementation. This consistency indicates that the newDecimalimplementation maintains compatibility and integrates properly within the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any differences in the Decimal class usage # Search for Decimal usage in the current file echo "Decimal usage in the current file:" grep -n "Decimal" packages/proto-codecs/src/codegen/osmosis/gamm/v1beta1/gov.ts # Compare with Decimal usage in other files echo "Decimal usage in other files:" grep -r "Decimal" packages/proto-codecs/src --exclude="*gov.ts"Length of output: 15706
packages/proto-codecs/src/codegen/osmosis/superfluid/superfluid.ts (1)
4-4: LGTM. Verify the custom Decimal implementation.The change from
@cosmjs/mathto a local../../decimalsmodule for theDecimalimport looks good. This suggests a move to a custom implementation, which can provide more control and potentially better alignment with the Osmosis protocol's needs.To ensure the new
Decimalimplementation is consistent with the previous one, please run the following verification:✅ Verification successful
Verified the
Decimalimplementation import.The
decimals.tsfile exists at the correct relative path (../../decimals.ts) as used in the import statement. The change from@cosmjs/mathto the local../../decimalsmodule is correctly implemented.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare the new Decimal implementation with the previous one from @cosmjs/math # Check if the new Decimal implementation exists if [ ! -f "$(dirname "$0")/../../decimals.ts" ]; then echo "Error: New Decimal implementation not found" exit 1 fi # Compare the interface of the new Decimal with @cosmjs/math Decimal diff <(grep -E '^(export interface|export class|export type)' "$(dirname "$0")/../../decimals.ts") <(grep -E '^(export interface|export class|export type)' "$(dirname "$0")/../../node_modules/@cosmjs/math/build/decimal.d.ts") || echo "Interfaces differ, please review carefully" # Check for any usage of @cosmjs/math Decimal in the codebase if grep -R "from \"@cosmjs/math\"" "$(dirname "$0")/../.."; then echo "Warning: @cosmjs/math is still being used in some files. Consider updating all occurrences." fiLength of output: 192
Script:
#!/bin/bash # Description: Locate all instances of decimals.ts in the codebase fd 'decimals\.ts$' .Length of output: 70
packages/proto-codecs/src/codegen/cosmos/staking/v1beta1/tx.ts (1)
Line range hint
516-519: Approve changes to MsgEditValidator and suggest test caseThe modifications to the
MsgEditValidatorinterface improve the handling of thecommissionRateproperty by using theDecimalclass consistently. This change ensures a standardized precision of 18 decimal places and provides explicit conversions between user input, atomic representation, and string output.To verify the correct implementation, please add a test case that covers the encoding and decoding of
MsgEditValidatorwith various commission rates. Here's a suggested test structure:describe('MsgEditValidator', () => { it('should correctly encode and decode commission rates', () => { const testCases = [ { input: '0.1', expected: '100000000000000000' }, { input: '0.01', expected: '10000000000000000' }, { input: '1', expected: '1000000000000000000' }, ]; for (const { input, expected } of testCases) { const msg: MsgEditValidator = { description: Description.fromPartial({}), validatorAddress: 'testAddress', commissionRate: input, minSelfDelegation: '', }; const encoded = MsgEditValidator.encode(msg).finish(); const decoded = MsgEditValidator.decode(encoded); expect(decoded.commissionRate).toBe(input); expect(Decimal.fromUserInput(input, 18).atomics).toBe(expected); } }); });This test will ensure that the encoding and decoding processes work correctly for various commission rate inputs.
Also applies to: 534-537
packages/proto-codecs/src/codegen/osmosis/superfluid/tx.ts (4)
Line range hint
1-1509: Overall code structure and implementation look goodThe file structure and implementation appear to be consistent and follow a clear pattern for defining message types and their associated methods. The main change in this file is the
Decimalimport, which has been addressed in previous comments.The overall implementation looks good, but please ensure that all verifications related to the
Decimalimport change are performed thoroughly. Additionally, consider adding unit tests specifically for the message types that use theDecimalclass to ensure their behavior remains correct after this change.
Line range hint
1267-1309: VerifyDecimalusage inMsgAddToConcentratedLiquiditySuperfluidPositionResponseSimilar to the previous message type, this one also uses the
Decimalclass in itsencodeanddecodemethods. We need to ensure that these operations are still correct with the newDecimalimport.Please verify that the
Decimaloperations in theencodeanddecodemethods ofMsgAddToConcentratedLiquiditySuperfluidPositionResponseare functioning correctly with the new import. Pay special attention to thefromUserInputandfromAtomicsmethods.
Line range hint
1024-1066: VerifyDecimalusage inMsgUnlockAndMigrateSharesToFullRangeConcentratedPositionResponseThe
encodeanddecodemethods of this message type use theDecimalclass. With the recent change in theDecimalimport, we need to ensure that these operations still work as expected.Please verify that the
Decimaloperations in theencodeanddecodemethods ofMsgUnlockAndMigrateSharesToFullRangeConcentratedPositionResponseare functioning correctly with the new import. Pay special attention to thefromUserInputandfromAtomicsmethods.
4-4: Verify compatibility of the newDecimalimportThe
Decimalimport has been changed from an external package to a local import. This change could potentially affect the behavior ofDecimaloperations throughout the file.Please ensure that the locally imported
Decimalclass is fully compatible with the previously used one from "@cosmjs/math". Verify that all usages ofDecimalin this file and any dependent files still work as expected.packages/proto-codecs/src/codegen/cosmos/staking/v1beta1/staking.ts (5)
Line range hint
1001-1009: Encoding changes forCommissionRateslook correct.The encoding of
rate,maxRate, andmaxChangeRatehas been updated to use the new localDecimalimplementation. The changes appear to be correct and consistent.To ensure that the new
Decimalimplementation behaves the same as the old one, please add unit tests that compare the encoding results of the old and new implementations:import { Decimal as OldDecimal } from "@cosmjs/math"; import { Decimal as NewDecimal } from "../../../decimals"; describe("CommissionRates encoding", () => { it("should produce the same encoding results with new Decimal implementation", () => { const testCases = [ { rate: "0.1", maxRate: "0.2", maxChangeRate: "0.01" }, { rate: "0.5", maxRate: "1.0", maxChangeRate: "0.1" }, // Add more test cases as needed ]; testCases.forEach(({ rate, maxRate, maxChangeRate }) => { const oldEncoding = encodeCommissionRatesWithOldDecimal(rate, maxRate, maxChangeRate); const newEncoding = CommissionRates.encode({ rate, maxRate, maxChangeRate, }).finish(); expect(newEncoding).toEqual(oldEncoding); }); }); }); function encodeCommissionRatesWithOldDecimal(rate: string, maxRate: string, maxChangeRate: string): Uint8Array { const writer = new BinaryWriter(); writer.uint32(10).string(OldDecimal.fromUserInput(rate, 18).atomics); writer.uint32(18).string(OldDecimal.fromUserInput(maxRate, 18).atomics); writer.uint32(26).string(OldDecimal.fromUserInput(maxChangeRate, 18).atomics); return writer.finish(); }This test will help ensure that the new implementation produces the same results as the old one.
Line range hint
1336-1336: Encoding change forValidator.delegatorShareslooks correct.The encoding of
delegatorSharesin theValidatortype has been updated to use the new localDecimalimplementation. The change appears to be correct and consistent with the other changes.To ensure that the new
Decimalimplementation behaves the same as the old one forValidatorencoding, please add the following test case to your unit tests:import { Decimal as OldDecimal } from "@cosmjs/math"; import { Decimal as NewDecimal } from "../../../decimals"; describe("Validator encoding", () => { it("should produce the same encoding results for delegatorShares with new Decimal implementation", () => { const testCases = [ "100", "1000000", "0.1", "999999999999.999999999999999999", // Add more test cases as needed ]; testCases.forEach((delegatorShares) => { const oldEncoding = encodeValidatorDelegatorSharesWithOldDecimal(delegatorShares); const newEncoding = Validator.encode({ delegatorShares, // Add other required fields with dummy values }).finish(); // Compare only the relevant part of the encoding (delegatorShares) expect(newEncoding.slice(0, oldEncoding.length)).toEqual(oldEncoding); }); }); }); function encodeValidatorDelegatorSharesWithOldDecimal(delegatorShares: string): Uint8Array { const writer = new BinaryWriter(); writer.uint32(50).string(OldDecimal.fromUserInput(delegatorShares, 18).atomics); return writer.finish(); }This test will help ensure that the new implementation produces the same results as the old one for the
Validator.delegatorSharesfield.
Line range hint
1-2270: Summary of changes and recommendationsThe changes in this file are part of a larger effort to replace the external
@cosmjs/mathDecimal implementation with a local one. The modifications appear to be consistent and correctly applied throughout the file. Here's a summary of the changes:
- Import of
Decimalchanged from@cosmjs/mathto a local implementation.- Updated encoding functions for
CommissionRates,Validator, andParamsto use the newDecimalimplementation.To ensure the correctness and consistency of these changes, please:
- Implement the suggested unit tests for each changed component to verify that the new
Decimalimplementation produces the same results as the old one.- Run the provided shell scripts to check for any remaining usages of the old
Decimalimplementation in the codebase.- Update the project's documentation to reflect this change in the
Decimalimplementation, including any potential impacts on other parts of the system that might interact with these staking-related structures.- Consider adding a comment in the file header explaining the reason for this change and any benefits it brings (e.g., performance improvements, better precision control, etc.).
As a final verification step, please run the following script to ensure that the new
Decimalimplementation is used consistently across all staking-related files:#!/bin/bash # Search for Decimal usage in staking-related files rg --type typescript 'Decimal\.fromUserInput' packages/proto-codecs/src/codegen/cosmos/stakingThis will help identify any staking-related files that might have been missed during the update process.
Line range hint
2270-2270: Encoding change forParams.minCommissionRatelooks correct.The encoding of
minCommissionRatein theParamstype has been updated to use the new localDecimalimplementation. This change is consistent with the other modifications in the file.To ensure that the new
Decimalimplementation behaves the same as the old one forParamsencoding, please add the following test case to your unit tests:import { Decimal as OldDecimal } from "@cosmjs/math"; import { Decimal as NewDecimal } from "../../../decimals"; describe("Params encoding", () => { it("should produce the same encoding results for minCommissionRate with new Decimal implementation", () => { const testCases = [ "0.01", "0.1", "0.05", "0.001", // Add more test cases as needed ]; testCases.forEach((minCommissionRate) => { const oldEncoding = encodeParamsMinCommissionRateWithOldDecimal(minCommissionRate); const newEncoding = Params.encode({ minCommissionRate, // Add other required fields with dummy values }).finish(); // Compare only the relevant part of the encoding (minCommissionRate) expect(newEncoding.slice(-oldEncoding.length)).toEqual(oldEncoding); }); }); }); function encodeParamsMinCommissionRateWithOldDecimal(minCommissionRate: string): Uint8Array { const writer = new BinaryWriter(); writer.uint32(50).string(OldDecimal.fromUserInput(minCommissionRate, 18).atomics); return writer.finish(); }This test will help ensure that the new implementation produces the same results as the old one for the
Params.minCommissionRatefield.Additionally, to verify that all instances of the old
Decimalhave been replaced, run the following script:#!/bin/bash # Search for any remaining usage of Decimal from @cosmjs/math rg --type typescript 'Decimal\.fromUserInput'If there are no results from the external
@cosmjs/mathlibrary, it confirms that all instances have been updated to use the new localDecimalimplementation.
6-6: Verify compatibility with the newDecimalimplementation.The import of
Decimalhas been changed from an external library (@cosmjs/math) to a local implementation (../../../decimals). This change could potentially affect the behavior of decimal operations throughout the module.To ensure compatibility, please run the following script to check for any other usages of the
@cosmjs/mathDecimal in the codebase:If there are no results, it confirms that the change has been applied consistently across the codebase. If there are results, those files may need to be updated as well to use the new local
Decimalimplementation.packages/proto-codecs/scripts/decimals-patch.ts (1)
11-98: Solid implementation of theDecimalclassThe
Decimalclass provides robust methods for handling decimal numbers according to the new requirements. Methods likefromUserInputandfromAtomicseffectively handle input validation and ensure compatibility with the updated Cosmos SDK. The overall design aligns well with the project's objectives.packages/proto-codecs/src/codegen/decimals.ts (3)
84-94: Confirm the need for negative number supportThe current implementation does not support negative numbers, as it only accepts non-negative integers. Verify whether negative decimal values are required for your use case.
If support for negative numbers is necessary, consider updating the regular expressions and logic to handle negative values appropriately.
44-44:⚠️ Potential issueEnsure correct handling when fractional part is all zeros
When the fractional part consists entirely of zeros, using
replace(/0+$/, "")results in an empty string. Confirm that this behavior is intentional and that subsequent padding correctly reconstructs the expected fractional digits.Consider adding a check to handle cases where the fractional part becomes empty after removing trailing zeros.
95-97: 🛠️ Refactor suggestionClarify the purpose of the
toString()methodThe
toString()method returns the atomic string representation of the number. Ensure this aligns with the expected behavior when converting aDecimalinstance to a string. If the intended use is to display the decimal in a human-readable format with fractional digits, additional formatting may be required.Consider implementing formatting logic if a human-readable decimal string is needed.
packages/trpc/src/concentrated-liquidity.ts (2)
1-12: LGTM!The import statements are correctly updated to include the necessary functions and utilities.
175-191: LGTM!The
getClParamsprocedure correctly retrieves and maps the concentrated liquidity parameters.packages/web/components/complex/pool/create/cl/set-base-info.tsx (1)
234-235: Verify the icon usage in 'TokenSelector' when no asset is selectedThe icon with ID
close-button-iconis being used and rotated by 45 degrees when no token is selected. Ensure that this is the intended icon and representation for an unselected state.Please confirm if the
close-button-iconis the appropriate choice here or if a different icon would better represent the "add token" action.packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/params.ts (2)
Line range hint
70-74: Ensure consistent decimal precision in encoding and decodingWhen encoding and decoding
authorizedSpreadFactorsandbalancerSharesRewardDiscount, the precision is set to 18 decimal places usingDecimal.fromUserInput(..., 18). Please verify that this precision matches the expected precision for these values within the system.Run the following script to check for consistency in the use of decimal precision across the codebase:
#!/bin/bash # Description: Verify consistent decimal precision # Search for Decimal conversions and list the precision used rg --type ts 'Decimal\.from(UserInput|Atomics)\(.*?,\s*\d+\)' -A 0 -B 0 --line-number # Expectation: All instances should use the same precision value (e.g., 18)Also applies to: 97-101
3-3: Verify compatibility of the new Decimal moduleThe import statement has been updated to import
Decimalfrom a local module"../../decimals"instead of"@cosmjs/math". Please ensure that the localDecimalmodule provides the same API and functionality as the original@cosmjs/mathmodule to prevent any unexpected behavior.You can run the following script to check if the local
Decimalmodule exports the expected methods:packages/proto-codecs/src/codegen/osmosis/cosmwasmpool/v1beta1/model/module_query_msg.ts (6)
Line range hint
141-143: Confirm updates inCalcInAmtGivenOutencoding ofswapFee.The
swapFeeinCalcInAmtGivenOutis also encoded usingDecimal.fromUserInput(message.swapFee, 18).atomics. Ensure that this change is appropriate and that all related components are updated accordingly.Extend the verification to include
CalcInAmtGivenOut:#!/bin/bash # Description: Verify `swapFee` handling in `CalcInAmtGivenOut` encoding. # Test: Search for `CalcInAmtGivenOut` encoding functions handling `swapFee`. rg --type typescript -A 5 'CalcInAmtGivenOut.*encode' packages | rg 'swapFee' -A 2 # Test: Check for use of `Decimal.fromUserInput` with `swapFee`. rg --type typescript -A 2 'swapFee' packages | rg 'Decimal\.fromUserInput'
Line range hint
157-159: Confirm updates inCalcInAmtGivenOutdecoding ofswapFee.The
swapFeeinCalcInAmtGivenOutis decoded usingDecimal.fromAtomics(reader.string(), 18).toString(). Verify the correctness of this implementation and consistency with the rest of the code.Extend the verification to include decoding in
CalcInAmtGivenOut:#!/bin/bash # Description: Verify `swapFee` handling in `CalcInAmtGivenOut` decoding. # Test: Search for `CalcInAmtGivenOut` decoding functions handling `swapFee`. rg --type typescript -A 5 'CalcInAmtGivenOut.*decode' packages | rg 'swapFee' -A 2 # Test: Check for use of `Decimal.fromAtomics` with `swapFee`. rg --type typescript -A 2 'swapFee' packages | rg 'Decimal\.fromAtomics'
Line range hint
85-87: Ensure consistent encoding ofswapFeeusingDecimal.The
swapFeeis now being encoded withDecimal.fromUserInput(message.swapFee, 18).atomics. Confirm that all inputs toswapFeeare correctly formatted as user input decimals and that this change doesn't introduce any discrepancies in fee calculations.Run the following script to identify where
swapFeeis used and ensure consistent handling:#!/bin/bash # Description: Find all instances of `swapFee` to verify its handling. # Test: Search for `swapFee` usage in encoding functions. rg --type typescript -A 5 'function.*encode.*\(.*swapFee' packages # Test: Check for consistent use of `Decimal.fromUserInput` with `swapFee`. rg --type typescript -A 2 'swapFee' packages | rg 'Decimal\.fromUserInput'
Line range hint
103-105: Ensure consistent decoding ofswapFeeusingDecimal.In the decode function,
swapFeeis now decoded usingDecimal.fromAtomics(reader.string(), 18).toString(). Verify that this conversion correctly handles the atomic representation and that it aligns with the encoding counterpart.Include in the previous script to check decoding functions:
#!/bin/bash # Description: Verify `swapFee` handling in decoding functions. # Test: Search for `swapFee` usage in decode functions. rg --type typescript -A 5 'function.*decode.*\(.*swapFee' packages # Test: Check for consistent use of `Decimal.fromAtomics` with `swapFee`. rg --type typescript -A 2 'swapFee' packages | rg 'Decimal\.fromAtomics'
Line range hint
49-51: Review the handling of optional properties in Amino conversions.In the
toAminomethods, properties liketokenOutDenomandswapFeeare conditionally set toundefinedif empty strings. Verify that this aligns with the expected Amino encoding requirements and that the optionality is correctly handled.Run the following script to check for consistency in Amino conversions:
#!/bin/bash # Description: Ensure that optional properties are correctly handled in Amino conversions. # Test: Search for `toAmino` methods and review property assignments. rg --type typescript -A 10 'toAmino\(message' packages | rg 'obj\.\w+ =' # Test: Confirm that properties are set to `undefined` when empty. rg --type typescript -A 2 '= message\.\w+ === "" \? undefined :' packagesAlso applies to: 181-183
8-8: Verify the new import path forDecimal.The
Decimalclass import path has been changed from@cosmjs/mathto a local module located at../../../../decimals. Please ensure that this localdecimalsmodule provides the necessaryDecimalimplementation compatible with the rest of the codebase. This change might affect other parts of the application relying onDecimal.Run the following script to check for the existence and export of the
Decimalclass in the local module:packages/proto-codecs/src/codegen/osmosis/cosmwasmpool/v1beta1/model/pool_query_msg.ts (1)
8-8: Update Import Statement forDecimalThe import path for
Decimalhas been updated to../../../../decimals. Ensure that this local implementation ofDecimalis functioning as intended and is compatible with the rest of the codebase.To verify that all imports of
Decimalare consistent across the codebase, you can run the following script:packages/proto-codecs/src/codegen/osmosis/poolmanager/v1beta1/genesis.ts (8)
8-8: Import of Decimal class updated correctlyThe
Decimalclass import has been updated to../../../decimals. Ensure that this new import path is correct and that theDecimalimplementation meets the requirements for handling decimal values throughout the codebase.
Line range hint
22-25: Verify precision and correctness in Decimal encodingIn the
encodefunction ofTakerFeeParams,defaultTakerFeeis now encoded usingDecimal.fromUserInput(message.defaultTakerFee, 18).atomics. Please confirm that:
- The precision of
18aligns with the expected decimal places fordefaultTakerFee.- Using
fromUserInputis appropriate for the input format ofdefaultTakerFee.- The
atomicsrepresentation is suitable for serialization.
Line range hint
30-32: Confirm accuracy of Decimal decodingIn the
decodefunction ofTakerFeeParams,defaultTakerFeeis decoded usingDecimal.fromAtomics(reader.string(), 18).toString(). Please ensure that:
- The precision
18matches the encoding precision.- The conversion properly reconstructs the original decimal value.
- There are no potential issues with number precision or rounding errors.
Line range hint
39-42: Ensure correct encoding of stakingRewardsIn the
encodefunction ofTakerFeeDistributionPercentage,stakingRewardsis encoded usingDecimal.fromUserInput(message.stakingRewards, 18).atomics. Please verify:
- The precision
18is appropriate forstakingRewards.- The input format for
stakingRewardsmatches the expectations offromUserInput.- The resulting
atomicsvalue is accurate for serialization.
Line range hint
43-47: Validate encoding of communityPool percentagesSimilarly,
communityPoolis encoded usingDecimal.fromUserInput(message.communityPool, 18).atomics. Confirm that:
- The precision
18aligns with the expected decimals forcommunityPool.- The use of
fromUserInputcorrectly handles the input value.- The serialization will accurately represent the intended percentage.
Line range hint
52-54: Check decoding process for stakingRewardsIn the
decodefunction,stakingRewardsis decoded usingDecimal.fromAtomics(reader.string(), 18).toString(). Please ensure:
- The precision matches the one used during encoding.
- The decoding accurately reconstructs the original
stakingRewardsvalue without loss of precision.
Line range hint
55-58: Confirm correctness of communityPool decodingFor
communityPool, verify that the decoding usingDecimal.fromAtomics(reader.string(), 18).toString():
- Uses the correct precision.
- Accurately restores the original value intended during encoding.
- Maintains consistency with
stakingRewardsdecoding.
Line range hint
22-25: Assess potential impact on backward compatibilityThe changes in encoding and decoding decimal values using
Decimal.fromUserInputandDecimal.fromAtomicsmay affect the serialization format. Please evaluate:
- Whether these changes maintain compatibility with existing data stored or transmitted.
- If clients or services interfacing with this code need updates to handle the new format.
- Any necessary data migrations or versioning to prevent disruptions.
Also applies to: 30-32, 39-47, 52-58
packages/stores/src/account/base.ts (2)
2-2: Consistent use of 'import { type ... }' syntaxThe change to use the
'import { type ... }'syntax for importing types improves consistency and aligns with TypeScript best practices introduced in TypeScript 3.8.Also applies to: 4-7
1074-1096: Ensure message normalization in all signing methodsThe message normalization logic added in the
signAminomethod addresses Decimal handling changes in Osmosis v26. Verify whether similar normalization is required in other signing methods likesignDirectandsignOneClickto ensure consistent transaction processing.Run the following script to check if message normalization is applied in other signing methods:
If the normalization logic is missing in these methods, consider adding it to handle Decimal changes consistently across all transaction signing processes.
packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/v1beta1/tx.ts (2)
1028-1030: Ensure correct usage ofDecimal.fromUserInputin encodingliquidityAmount.The
liquidityAmountis now encoded usingDecimal.fromUserInput(message.liquidityAmount, 18).atomics. Verify that:
- The
Decimalclass from../../../decimalscorrectly handles the conversion as expected.- The scale parameter
18is appropriate for the precision required forliquidityAmount.Run the following script to confirm consistent usage of
Decimal.fromUserInputand the scale parameter across the codebase:#!/bin/bash # Description: Find all usages of `Decimal.fromUserInput` to ensure consistent scale. # Expected result: All usages should use the correct scale (e.g., `18`). rg --type ts 'Decimal\.fromUserInput\(' -A 0
8-8: Confirm the compatibility of the localDecimalimport.The
Decimalclass is now imported from a local path../../../decimalsinstead of from the external package@cosmjs/math. Ensure that this localDecimalimplementation provides all the necessary functionality and remains consistent with other parts of the codebase that rely onDecimal.Run the following script to check for inconsistent imports of
Decimal:packages/proto-codecs/src/codegen/osmosis/poolmanager/v1beta1/tx.ts (1)
8-8: LGTM: Updated Decimal import pathThe import of
Decimalfrom"../../../decimals"is appropriate and aligns with the changes described. This ensures consistency in how decimal values are handled throughout the codebase.
packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/v1beta1/incentive_record.ts
Show resolved
Hide resolved
packages/proto-codecs/src/codegen/osmosis/concentratedliquidity/v1beta1/gov.ts
Show resolved
Hide resolved
| <SkeletonLoader className="h-[92px] w-[260px] rounded-3xl" /> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance loading state for 'TokenSelector' components
Currently, when the tokens are loading, a SkeletonLoader is displayed. Consider adding a brief descriptive text or spinner to inform users that the tokens are loading, enhancing the user experience.
Suggestion:
{!baseTokens ? (
<div className="flex flex-col items-center">
<SkeletonLoader className="h-[92px] w-[260px] rounded-3xl" />
<span>{t("pools.loadingBaseTokens")}</span>
</div>
) : (
// existing TokenSelector
)}Also applies to: 98-99
| "flex h-13 w-[520px] items-center justify-center gap-2.5 rounded-xl bg-wosmongton-700 transition-all hover:bg-wosmongton-800 focus:bg-wosmongton-900 disabled:pointer-events-none disabled:bg-osmoverse-500" | ||
| )} | ||
| <Button | ||
| disabled={isTxLoading || !isAgreementChecked || !selectedBase} |
There was a problem hiding this comment.
Include 'selectedQuote' in the 'Create Pool' button's disabled condition
The 'Create Pool' button should be disabled when either the base token or the quote token is not selected. Currently, it only checks for selectedBase. This could lead to runtime errors if the user attempts to create a pool without selecting a quote token.
Suggestion:
<Button
disabled={
isTxLoading ||
!isAgreementChecked ||
!selectedBase ||
+ !selectedQuote
}
isLoading={isLoadingBaseTokens || isLoadingQuoteTokens}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| disabled={isTxLoading || !isAgreementChecked || !selectedBase} | |
| disabled={isTxLoading || !isAgreementChecked || !selectedBase || !selectedQuote} |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/web/components/complex/pool/create/cl/set-base-info.tsx (4)
11-11: Improved imports and API usage.The changes to imports and the introduction of new API calls using TRPC are good improvements. They streamline the code and centralize data fetching.
However, consider grouping related imports together for better readability. For example:
import { SkeletonLoader, Spinner } from "~/components/loaders"; import { Button } from "~/components/ui/button"; import { useDisclosure, useFilteredData, useTranslation } from "~/hooks"; import { api } from "~/utils/trpc";Also applies to: 14-14, 18-20, 24-24, 50-55
50-55: Improved state management with new API structure.The introduction of new state variables for API data and the removal of memoization for
baseTokensandquoteTokensalign well with the new API structure. This change simplifies the component's logic.However, consider the performance implications of removing memoization. If the token lists are large or frequently accessed, you might want to reintroduce memoization:
const memoizedBaseTokens = React.useMemo(() => baseTokens, [baseTokens]); const memoizedQuoteTokens = React.useMemo(() => quoteTokens, [quoteTokens]);
69-99: Improved UI rendering with loading states and translations.The addition of loading states using SkeletonLoader and the use of translation functions are excellent improvements. They enhance both user experience and internationalization support.
To further improve the loading state, consider adding a brief text to inform users what's loading:
{!baseTokens ? ( <div className="flex flex-col items-center"> <SkeletonLoader className="h-[92px] w-[260px] rounded-3xl" /> <span className="text-sm text-osmoverse-400">{t("loading.baseTokens")}</span> </div> ) : ( // ... existing TokenSelector )}Apply this change to both base and quote token selectors.
Line range hint
148-186: Improved create pool button with better error handling and translations.The updates to the create pool button, including improved error handling, pool ID extraction, and the use of translation functions, are excellent improvements. They enhance the component's functionality and user experience.
However, the button's disabled state should also consider the quote token selection:
disabled={!isAgreementChecked || !selectedBase || !selectedQuote}This change will prevent users from attempting to create a pool without selecting both tokens.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/proto-codecs/scripts/tests/decimal-patch.spec.ts (1 hunks)
- packages/stores/src/account/amino-converters.ts (1 hunks)
- packages/stores/src/account/base.ts (4 hunks)
- packages/web/components/complex/pool/create/cl/set-base-info.tsx (7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/proto-codecs/scripts/tests/decimal-patch.spec.ts
- packages/stores/src/account/base.ts
🧰 Additional context used
🔇 Additional comments (5)
packages/web/components/complex/pool/create/cl/set-base-info.tsx (2)
Line range hint
103-117: Improved SpreadSelector with API integration and loading state.The updates to the SpreadSelector component, including the use of data from the new API and the addition of a loading state, are well-implemented. These changes enhance the component's reliability and user experience.
The loading state effectively prevents potential errors when
clParamsare not yet available. Good job on considering this edge case.
Line range hint
1-324: Overall assessment: Significant improvements with minor suggestionsThis refactoring has greatly improved the
SetBaseInfoscomponent by integrating a new API structure, enhancing UI with better loading states, improving internationalization, and strengthening error handling. The changes align well with the PR objectives of addressing transaction issues and standardizing data types.Key improvements:
- Integration with new API calls using TRPC
- Enhanced loading states for better user experience
- Improved internationalization with translation functions
- Better error handling in the create pool functionality
Suggestions for further improvement:
- Group related imports for better readability
- Consider reintroducing memoization for large token lists
- Add descriptive text to loading states
- Include quote token selection in the create pool button's disabled state
These changes significantly contribute to the reliability of transaction processing within the Osmosis platform, addressing the main objectives of the pull request.
packages/stores/src/account/amino-converters.ts (3)
2-4: Importing necessary types forMsgBeginUnlockingThe import statement correctly adds
MsgBeginUnlockingandMsgBeginUnlockingAminofrom the appropriate module.
Line range hint
26-33: Verify the handling of emptycoinsintoAminoforMsgBeginUnlockingIn the custom
toAminofunction for/osmosis.lockup.MsgBeginUnlocking, you deleteobj.coinswhenobj.coins?.length === 0. Please verify that removing this field when it's an empty array does not cause issues with downstream processing or expectations. Some systems might expect thecoinsfield to be present even if it's empty.
Line range hint
49-76: Exclusion oftimeout_timestampinMsgTransferis appropriateThe
toAminofunction for/ibc.applications.transfer.v1.MsgTransfercorrectly excludestimeout_timestamp, as it's not used in your transactions.
What is the purpose of the change:
This PR resolves transaction issues related to Decimal changes introduced in Osmosis v26. The fix involves encoding messages to the proto format, which normalizes data types like Decimals. Afterward, the messages are converted to their amino representations.
The following transactions are now correctly handled:
Linear Task
https://linear.app/osmosis/issue/FE-1130/fix-create-pools-tx
Testing and Verifying