refactor: update staking contract config structure #134
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of the staking contract, focusing on enhancing configuration management and validation. The changes involve restructuring the configuration data types, introducing more granular and type-safe configurations for native and protocol chains, and implementing robust validation mechanisms. The modifications span across multiple files, including state management, message handling, migration logic, and test suites, with a primary goal of improving the contract's flexibility, modularity, and error handling. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Contract
participant Storage
Client->>Contract: Instantiate(native_chain_config, protocol_chain_config)
Contract->>Contract: Validate Configurations
alt Invalid Configuration
Contract-->>Client: Error
else Valid Configuration
Contract->>Storage: Save Configuration
Storage-->>Contract: Configuration Saved
Contract-->>Client: Instantiation Success
end
Client->>Contract: UpdateConfig(new_config)
Contract->>Contract: Validate New Configuration
alt Invalid Update
Contract-->>Client: Error
else Valid Update
Contract->>Storage: Update Configuration
Storage-->>Contract: Configuration Updated
Contract-->>Client: Update Success
end
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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: 2
🔭 Outside diff range comments (1)
contracts/staking/src/tests/test_helper.rs (1)
Line range hint
52-67: Add negative test cases.The
init()function only tests the happy path. Consider adding helper functions for negative test cases.pub fn init_with_invalid_config() -> Result<OwnedDeps<MockStorage, MockApi, MockQuerier>, ContractError> { let mut deps = mock_dependencies(); let mut msg = mock_init_msg(); msg.native_chain_config.validators = vec![]; // Invalid empty validators let info = mock_info(OSMO3, &coins(1000, "uosmo")); instantiate(deps.as_mut(), mock_env(), info, msg).map(|_| deps) }
🧹 Nitpick comments (18)
contracts/staking/src/types.rs (1)
15-22: Potential upper bound check ondao_treasury_fee.
Sincedao_treasury_feeis represented asUint128and used as a fraction (x/100000), consider adding a constraint (e.g., ≤ 100000) to avoid misconfigurations that exceed 100%.pub struct UnsafeProtocolFeeConfig { - pub dao_treasury_fee: Uint128, + /// Must be <= 100000 to represent <= 100% + pub dao_treasury_fee: Uint128, pub treasury_address: Option<String>, }contracts/staking/src/state.rs (2)
9-33: Ensure consistent usage of optional vs. mandatory fields.Overall, the expanded
Configstruct is well-structured, separating chain and fee logic. However, consider whether you truly need a non-optionalmonitorsvector and a mandatorystoppedboolean. For instance, certain logic might benefit from anOption<Vec<Addr>>to differentiate between no monitors vs. an empty set. Thestoppedfield could be replaced by a small enum if you plan to add partial or read-only modes in the future.
83-92: Use a more explicit representation for fees.Storing fees as a plain
Uint128(interpreted as x/100000) might be confusing for future maintainers. Consider using a fraction or a typed struct that clarifies the base multiplier, or aDecimaltype for immediate clarity.contracts/staking/src/tests/query_tests.rs (1)
19-62: Thorough coverage of new config fields in tests.The additions to
get_configadequately validate all relevant fields innative_chain_configandprotocol_chain_config. As a future enhancement, consider testing boundary conditions (e.g., empty validator lists, extremely largeminimum_liquid_stake_amount) to ensure robust behavior.contracts/staking/src/contract.rs (1)
168-172: Refactorupdate_configfor modular changes.Update messages can become unwieldy if too many fields are grouped together. Consider creating smaller specialized update functions (e.g.,
update_fee_config) to reduce potential mistakes and keep changes more targeted.contracts/staking/src/execute.rs (1)
725-725: Blank line
Minor layout change, no functional effect.contracts/staking/src/migrations/states/v0_4_18.rs (2)
28-28: Enhance documentation for fee calculation.The comment about fee percentage calculation could be more explicit.
- pub dao_treasury_fee: Uint128, // not using a fraction, fee percentage=x/100000 + pub dao_treasury_fee: Uint128, // Fee percentage is calculated as (fee_value/100000)*100%. Example: 10000 = 10%
20-22: Consider consolidating oracle address fields.Multiple optional oracle address fields suggest an evolving contract. Consider consolidating these into a more structured approach.
- pub oracle_contract_address: Option<Addr>, - pub oracle_contract_address_v2: Option<Addr>, - pub oracle_address: Option<Addr>, + pub oracle_config: OracleConfig, // Add this struct: +#[cw_serde] +pub struct OracleConfig { + pub legacy_address: Option<Addr>, // Previously oracle_contract_address + pub current_address: Option<Addr>, // Previously oracle_address + pub v2_address: Option<Addr>, // Previously oracle_contract_address_v2 +}contracts/staking/src/tests/test_helper.rs (1)
Line range hint
11-22: Organize test constants better.Consider grouping related constants and adding documentation about their purpose.
+// Test addresses for Osmosis chain pub static OSMO1: &str = "osmo12z558dm3ew6avgjdj07mfslx80rp9sh8nt7q3w"; pub static OSMO2: &str = "osmo13ftwm6z4dq6ugjvus2hf2vx3045ahfn3dq7dms"; pub static OSMO3: &str = "osmo1sfhy3emrgp26wnzuu64p06kpkxd9phel8ym0ge"; pub static OSMO4: &str = "osmo17x4zm0m0mxc428ykll3agmehfrxpr5hqpmsatd"; + +// Test addresses for Celestia chain pub static CELESTIA1: &str = "celestia1sfhy3emrgp26wnzuu64p06kpkxd9phel74e0yx"; pub static CELESTIA2: &str = "celestia1ztrhpdznu2xlwakd4yp3hg9lwyr3d46ayd30u2"; + +// Test validator addresses pub static CELESTIAVAL1: &str = "celestiavaloper1463wx5xkus5hyugyecvlhv9qpxklz62kyhwcts";contracts/staking/src/ibc.rs (2)
Line range hint
36-42: Reduce code duplication in channel validation.The channel validation logic is duplicated between
receive_ackandreceive_timeout. Consider extracting it to a helper function.+fn validate_source_channel(config: &Config, source_channel: &str) -> Result<Response, ContractError> { + if source_channel != config.protocol_chain_config.ibc_channel_id { + return Ok(Response::new() + .add_attribute("action", "ibc_receive") + .add_attribute("channel", source_channel) + .add_attribute("error", "received packet for different channel")); + } + Ok(Response::new().add_attribute("action", "ibc_receive")) +}Also applies to: 75-80
Line range hint
91-94: Enhance error attributes for timeout handling.The timeout error attributes could be more informative by including the sequence number.
- Ok(response.add_attribute("error", "ibc packet timed out")) + Ok(response + .add_attribute("error", "ibc packet timed out") + .add_attribute("sequence", sequence.to_string()) + .add_attribute("channel", source_channel))contracts/staking/src/tests/instantiate_tests.rs (2)
12-105: Consider reducing code duplication in test functions.The test functions for invalid configurations follow a similar pattern. Consider introducing a helper function to reduce code duplication.
Example refactor:
fn test_invalid_config<F>(modify_config: F) where F: FnOnce(&mut InstantiateMsg), { let mut deps = mock_dependencies(); let info = mock_info(OSMO3, &[]); let mut msg = mock_init_msg(); modify_config(&mut msg); let res = crate::contract::instantiate( deps.as_mut(), cosmwasm_std::testing::mock_env(), info.clone(), msg, ); assert!(res.is_err()); } #[test] fn invalid_native_chain_account_address_prefix_fails() { test_invalid_config(|msg| { msg.native_chain_config.account_address_prefix = "".to_string(); }); }
203-259: Enhance test assertions with descriptive messages.The test assertions in
init_properlycould be more descriptive to help identify failures quickly.Example enhancement:
- assert!(pending_batch.id == 1); + assert_eq!(pending_batch.id, 1, "First batch ID should be 1"); - assert_eq!( + assert_eq!( NativeChainConfig { token_denom: "utia".to_string(), // ... }, config.native_chain_config, + "Native chain config should match expected values" );contracts/staking/src/helpers.rs (3)
8-41: Enhance address prefix validation.The address prefix validation is good but could be more robust:
- Consider adding a maximum length check based on the Bech32 specification
- Consider validating against a list of known prefixes
pub fn validate_address_prefix(hrp: &str) -> StdResult<String> { - if hrp.is_empty() || hrp.len() > 83 { + // As per Bech32 spec, HRP length should be 1-83 chars + const MIN_HRP_LENGTH: usize = 1; + const MAX_HRP_LENGTH: usize = 83; + + if hrp.len() < MIN_HRP_LENGTH || hrp.len() > MAX_HRP_LENGTH { return Err(StdError::generic_err("invalid address prefix length")); } let mut has_lower: bool = false; let mut has_upper: bool = false; for b in hrp.bytes() { - // Valid subset of ASCII - if !(33..=126).contains(&b) { + // As per Bech32 spec, HRP chars must be ASCII chars between 33-126 + if !matches!(b, 33..=126) { return Err(StdError::generic_err( "address prefix contains invalid chars", )); }
203-215: Strengthen denom validation.The denom validation could be more comprehensive:
- Add a maximum length check
- Consider validating against a specific pattern
- Add validation for common prefixes like 'u' for micro
pub fn validate_denom(denom: impl Into<String>) -> StdResult<String> { let denom: String = denom.into(); - if denom.len() <= 3 { - return Err(StdError::generic_err("denom len is less than 3")); + const MIN_DENOM_LENGTH: usize = 3; + const MAX_DENOM_LENGTH: usize = 128; + + if denom.len() < MIN_DENOM_LENGTH { + return Err(StdError::generic_err( + format!("denom length must be at least {}", MIN_DENOM_LENGTH) + )); + } + if denom.len() > MAX_DENOM_LENGTH { + return Err(StdError::generic_err( + format!("denom length must not exceed {}", MAX_DENOM_LENGTH) + )); } - if !denom.chars().all(|c| c.is_ascii_alphabetic()) { - return Err(StdError::generic_err("denom must be alphabetic")); + + // Common prefix validation + if denom.starts_with('u') && denom.len() > 1 { + let unit = &denom[1..]; + if !unit.chars().all(|c| c.is_ascii_alphabetic()) { + return Err(StdError::generic_err( + "micro unit must contain only alphabetic characters" + )); + } + } else if !denom.chars().all(|c| c.is_ascii_alphabetic()) { + return Err(StdError::generic_err( + "denom must contain only alphabetic characters" + )); } Ok(denom)
217-226: Improve IBC denom validation.The IBC denom validation could be more flexible and informative:
- Add validation for the hash portion
- Consider supporting different IBC denom formats
- Provide more specific error messages
pub fn validate_ibc_denom(ibc_denom: impl Into<String>) -> StdResult<String> { let ibc_denom: String = ibc_denom.into(); - if ibc_denom.starts_with("ibc/") && ibc_denom.strip_prefix("ibc/").unwrap().len() == 64 { - Ok(ibc_denom) - } else { - Err(StdError::generic_err("ibc denom is invalid")) + const IBC_PREFIX: &str = "ibc/"; + const HASH_LENGTH: usize = 64; + + if !ibc_denom.starts_with(IBC_PREFIX) { + return Err(StdError::generic_err( + format!("ibc denom must start with '{}'", IBC_PREFIX) + )); + } + + let hash = ibc_denom.strip_prefix(IBC_PREFIX).unwrap(); + if hash.len() != HASH_LENGTH { + return Err(StdError::generic_err( + format!("ibc denom hash must be {} characters long", HASH_LENGTH) + )); + } + + if !hash.chars().all(|c| c.is_ascii_hexdigit()) { + return Err(StdError::generic_err( + "ibc denom hash must be a valid hex string" + )); } + + Ok(ibc_denom)contracts/staking/src/tests/unstake_tests.rs (1)
220-220: Simplify batch state updates.The batch state updates can be simplified by chaining the operations.
- batch.update_status(BatchStatus::Pending, Some(env.block.time.seconds() - 1)); - BATCHES.save(&mut deps.storage, 1, &batch).unwrap(); + BATCHES.save(&mut deps.storage, 1, &batch.update_status(BatchStatus::Pending, Some(env.block.time.seconds() - 1))).unwrap();Also applies to: 227-227, 233-233
contracts/staking/src/tests/update_config_tests.rs (1)
1-431: Comprehensive test coverage for configuration updates.The test suite thoroughly covers configuration validation scenarios. However, consider adding the following test cases:
- Test for concurrent updates of multiple configurations
- Test for empty/null optional fields
- Test for maximum/minimum bounds of numeric fields
Example test case for concurrent updates:
#[test] fn update_multiple_configs_properly() { let mut deps = init(); let info = cosmwasm_std::testing::mock_info(OSMO3, &[]); let config_update_msg = crate::msg::ExecuteMsg::UpdateConfig { native_chain_config: Some(UnsafeNativeChainConfig { // ... native chain config fields }), protocol_chain_config: Some(UnsafeProtocolChainConfig { // ... protocol chain config fields }), protocol_fee_config: Some(UnsafeProtocolFeeConfig { // ... protocol fee config fields }), batch_period: Some(100u64), monitors: Some(vec![OSMO1.to_string()]), }; let res = crate::contract::execute( deps.as_mut(), cosmwasm_std::testing::mock_env(), info.clone(), config_update_msg, ); assert!(res.is_ok()); let config = CONFIG.load(&deps.storage).unwrap(); // Assert all configurations are updated correctly }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
contracts/staking/Cargo.toml(1 hunks)contracts/staking/src/contract.rs(8 hunks)contracts/staking/src/error.rs(1 hunks)contracts/staking/src/execute.rs(19 hunks)contracts/staking/src/helpers.rs(2 hunks)contracts/staking/src/ibc.rs(2 hunks)contracts/staking/src/lib.rs(1 hunks)contracts/staking/src/migrations/mod.rs(1 hunks)contracts/staking/src/migrations/states/mod.rs(1 hunks)contracts/staking/src/migrations/states/v0_4_18.rs(1 hunks)contracts/staking/src/migrations/states/v0_4_20.rs(1 hunks)contracts/staking/src/migrations/v0_4_20.rs(2 hunks)contracts/staking/src/migrations/v1_0_0.rs(1 hunks)contracts/staking/src/msg.rs(5 hunks)contracts/staking/src/query.rs(1 hunks)contracts/staking/src/state.rs(1 hunks)contracts/staking/src/tests/circuit_breaker_tests.rs(2 hunks)contracts/staking/src/tests/helper_tests.rs(1 hunks)contracts/staking/src/tests/instantiate_tests.rs(1 hunks)contracts/staking/src/tests/mod.rs(1 hunks)contracts/staking/src/tests/query_tests.rs(1 hunks)contracts/staking/src/tests/reward_tests.rs(4 hunks)contracts/staking/src/tests/stake_tests.rs(1 hunks)contracts/staking/src/tests/test_helper.rs(2 hunks)contracts/staking/src/tests/unstake_tests.rs(2 hunks)contracts/staking/src/tests/update_config_tests.rs(1 hunks)contracts/staking/src/tests/withdraw_tests.rs(4 hunks)contracts/staking/src/types.rs(1 hunks)scripts/testnet/init-stake-contract.sh(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- contracts/staking/src/lib.rs
- contracts/staking/src/tests/mod.rs
- contracts/staking/src/migrations/states/mod.rs
- contracts/staking/src/tests/helper_tests.rs
🧰 Additional context used
📓 Learnings (1)
contracts/staking/src/msg.rs (1)
Learnt from: manu0466
PR: milkyway-labs/milkyway-contracts#129
File: contracts/treasury/src/msg.rs:73-73
Timestamp: 2024-11-12T02:30:53.601Z
Learning: The `MigrateMsg` in `contracts/treasury/src/msg.rs` is intentionally designed as a struct instead of an enum due to the lack of specific migration logic that needs to be enforced.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Suite
🔇 Additional comments (68)
contracts/staking/src/migrations/v1_0_0.rs (5)
13-20: Ensure consistent migration parameter order in MigrateMsg.
All parameters in the migration function align with the newMigrateMsg::V0_4_20ToV1_0_0fields. Confirm that no parameter ordering mismatches exist between the message variant and this function signature, which might cause confusion or potential runtime deserialization issues.
21-23: Good practice: version check.
It’s good to see you verifying the contract version before proceeding with migration. This ensures that the migration occurs only if the existing stored version matches the expectedFROM_VERSION.
24-30: Comprehensive validation of prefixes and denom.
The validations for address prefixes and the native token denomination reduce the risk of invalid contract states. Well done.Also applies to: 31-32
34-61: Validating old configuration's addresses.
Ensuring that old addresses align with newly supplied prefixes mitigates potential address format conflicts after migration. This thorough validation step is crucial for preventing errors during contract upgrades.
63-93: Migration logic properly translates old config to new config.
The creation ofConfigusingNativeChainConfig,ProtocolChainConfig, andProtocolFeeConfigis handled cleanly. This approach avoids data loss while allowing extension of future fields.Also applies to: 95-101
contracts/staking/src/types.rs (3)
24-35: Validation method ensures seamless safe struct conversion.
The logic for converting thetreasury_addressinto a validated address is robust. Keeping the validation here centralizes correctness checks and clarifies the code flow for external references.
37-63: Clear separation of concerns inUnsafeNativeChainConfig.
By requiring explicit calls tovalidate(), you decouple raw input handling from secure data usage. This pattern fosters safer expansions of chain-specific logic.
81-125: IBC channel ID format check is valuable.
Validating that theibc_channel_idstarts with"channel-"and parsing the numeric suffix prevents accidental misuse of incorrectly formatted channel IDs. This is a good safeguard.contracts/staking/src/msg.rs (4)
16-24: Enhanced instantiation with structured configs.
EmbeddingUnsafeNativeChainConfig,UnsafeProtocolChainConfig, andUnsafeProtocolFeeConfiginInstantiateMsgpromotes a clearer and more extensible approach, reducing the risk of misconfigured fields at instantiation.
51-55: Partial updates with optional fields.
Allowing selective updates to each config category viaOption<T>is convenient and flexible. However, ensure that partial updates do not revert unchanged fields to defaults. Properly handleNoneto preserve existing values.
76-81: ConsistentConfigResponserefactor.
Refactoring to return the newNativeChainConfig,ProtocolChainConfig, andProtocolFeeConfigmakes it simpler for consumers to retrieve the entire config in a unified structure.
175-183: Comprehensive migration variant.
TheV0_4_20ToV1_0_0variant lines up well with the parameters in your migration function. This ensures a straightforward upgrade path for both user expectations and on-chain transformations.contracts/staking/src/state.rs (2)
35-61: Validate minimum requirements and edge cases forNativeChainConfig.The fields in
NativeChainConfigappear properly typed. However, consider clarifying or enforcing constraints, such as ensuringvalidatorsis not empty to avoid issues with distributing delegations. You might also want to confirm thatunbonding_periodis non-zero or within an acceptable range.
63-78: Confirm the intended usage oforacle_address.Allowing
oracle_addressto beNonegives flexibility. However, if the contract logic relies on an oracle for rate calculation or redemption, ensure that all code paths handle the case whereoracle_addressisNone. This would avoid panic paths or unexpected reverts.contracts/staking/src/tests/query_tests.rs (1)
8-12: Imports look appropriate.Adding
CELESTIA1,CELESTIA2, andOSMO4as reused constants is consistent with the test harness. IncludingAddrfromcosmwasm_stdlooks correct for the new usage in test checks.contracts/staking/src/contract.rs (6)
Line range hint
5-26: Import and usage alignment confirmed.The added imports (
validate_addresses,validate_denom,Config,State, etc.) consistently reflect the newly introduced validation and state management logic. No issues spotted.
58-75: Validate and construct newConfigaccurately during instantiation.Using
validate()methods on all config structures is a good defensive design. Ensure these validations include checks for required prefixes, acceptable denom formats, and thatliquid_stake_token_denomis sanitized. The defaultstopped = trueapproach is suitable for a “safe by default” design.
136-136: Inevitably couplingmust_paywithprotocol_chain_config.ibc_token_denom.Confirm that the entire flow of tokens in the contract now expects IBC-based tokens rather than the older
native_token_denom. Be sure to remove any remaining references to the old denom to avoid confusion.
159-163: Ensure partial config updates are consistent.When updating the config, confirm that none of these new fields inadvertently reset existing fields to defaults (e.g., overwriting addresses to
Noneor blank). Proper null checks or a partial update pattern might help preserve existing configurations that are not being updated.
262-262: Safe migration logic to prevent downgrades.The safeguard ensures no unintended rollback in contract versions. Confirm that integrators are aware they cannot revert to older contract states.
275-287: Migration to v1.0.0 is well-defined.Migrating to the new config structure is clearly laid out, carrying forward the essential prefix and denom details. Double-check that older deployments will seamlessly transition if they have partial or missing fields.
contracts/staking/src/execute.rs (31)
1-1: Imports for new config structures and utilities
These additional imports align well with the updated configuration approach. No issues detected.Also applies to: 10-10, 14-14, 16-16
36-36: Check for emptyibc_channel_id
This early return provides clear error handling. Looks good.
41-41: Referencingconfig.protocol_chain_config.ibc_token_denom
Straightforward usage. No concerns.
49-49: Ensurestaker_addressis valid
This line referencesconfig.native_chain_config.staker_address. Consider validating non-emptiness similarly toibc_channel_idif there's a possibility it could be missing.
51-51: Use ofconfig.protocol_chain_config.ibc_channel_id
Consistent with the earlier check at line 36.
110-115: Unwrapping the optionaloracle_address
Calling.unwrap()may panic iforacle_addressis unexpectedlyNone. Confirm in upstream logic that an oracle address is always set before reaching this code.
164-164: Check minimum liquid stake amount
The validation logic looks correct for comparing the sent amount with the minimum requirement.Also applies to: 166-166, 167-167
391-391: Use ofnative_chain_config.unbonding_period
Correctly calculates the expected time for native chain unbonding.
450-450: Cloning IBC token denom
Sufficient for constructing the message. No performance concerns.
475-478: Validate new validator address
Passingconfig.native_chain_config.validator_address_prefixtovalidate_addressis correct.
482-482: Avoid duplicate validators
The check ensures no repeated entries. Implementation is solid.
493-496: Add new validator
Straightforward addition to thevalidatorsvector.
516-519: Validate address for removal
Reuses the same prefix logic as adding a validator, which is consistent.
523-523: Find position of validator
The.position(...)callback is properly used to locate the validator.
529-529: Remove validator from list
Implementation is consistent and error handling is already present if not found.
708-710: Expandingupdate_configsignature
AcceptingOption<UnsafeNativeChainConfig>,Option<UnsafeProtocolChainConfig>, andOption<UnsafeProtocolFeeConfig>promotes flexible partial updates.
712-712: Addition of monitors
Allowing an optional list of addresses broadens monitoring capabilities.
722-723: Update protocol chain config
Properly validated upon assignment.
727-727: Update protocol fee config
The.validatemethod ensures correctness relative to the protocol chain config.
730-734: Validate monitor addresses
Ensures each monitor address uses the correct prefix.
737-738: Updatebatch_period
Straightforward assignment.
757-759: Derive intermediate sender
Deploys newly structured fields fromprotocol_chain_configandnative_chain_config.
775-775: Find correct IBC coin
Matches the token denom from config.
797-797: Fee handling logic
Falls back to the contract’stotal_feesiftreasury_addressis unset.
1023-1031: Ensure treasury is configured before withdrawal
The unwrap is safe thanks to the precedingis_none()check.
1037-1037: Cloning treasury address
No concerns.
1039-1039: Setting denom insend_msg
Uses config-provided IBC denom. No issues.
1046-1046: Attachreceiverattribute
Useful for traceability.
839-841: Intermediate sender forreceive_unstaked_tokens
Again leveragesprotocol_chain_configandnative_chain_config. Consistent usage.
857-857: Locate IBC coin for unstaked tokens
Same approach as in reward logic. No issues.
904-904: Check monitors forcircuit_breaker
Allows either admin or a monitor to halt the contract. No concerns.contracts/staking/src/migrations/mod.rs (1)
1-1: Addition of new migration modules
Introducingstatesandv1_0_0mirrors the versioning approach. Looks well-structured.Also applies to: 3-3
contracts/staking/src/migrations/states/v0_4_20.rs (1)
1-27: New file definingv0_4_20::Config
A comprehensive configuration setup that includes addresses, token denoms, periods, and flags. The usage ofcw_serdeandItem<Config>is consistent with standard patterns.contracts/staking/src/migrations/v0_4_20.rs (1)
Line range hint
18-34: Add validation during migration.The migration copies fields directly without validation. Consider adding validation for critical fields like addresses and denominations.
Example validation:
fn validate_migration_config(config: &Config) -> ContractResult<()> { validate_address(&config.treasury_address)?; validate_denom(&config.native_token_denom)?; // Add more validations Ok(()) }contracts/staking/src/error.rs (1)
136-137: LGTM! The new error variant is well-defined.The
TreasuryNotConfigurederror variant is clear, follows the established pattern, and enhances configuration validation.contracts/staking/src/tests/circuit_breaker_tests.rs (1)
65-67: LGTM! Configuration access patterns updated correctly.The changes properly reflect the new configuration structure, maintaining the same test logic while using the reorganized config fields.
Also applies to: 74-74, 86-86
contracts/staking/src/tests/reward_tests.rs (2)
29-31: LGTM! Configuration access updated correctly inreceive_rewards.The changes properly reflect the new configuration structure for sender derivation, token denomination, and treasury handling.
Also applies to: 50-50, 58-58, 65-65
112-114: LGTM! Configuration access updated correctly inreceive_rewards_and_send_fees_to_treasury.The changes properly reflect the new configuration structure while maintaining the same test coverage and assertions.
Also applies to: 122-122, 148-152
contracts/staking/src/query.rs (1)
19-21: LGTM! Config response structure updated appropriately.The changes properly reflect the new configuration structure, improving modularity while maintaining the same query functionality.
Also applies to: 23-23
contracts/staking/src/tests/instantiate_tests.rs (1)
1-11: LGTM! Well-organized imports.The imports are properly organized and include all necessary dependencies for testing.
contracts/staking/src/tests/withdraw_tests.rs (1)
84-84: LGTM! Consistent configuration access.The changes correctly update the token denomination access to use
protocol_chain_config.ibc_token_denomfrom the new configuration structure.Also applies to: 125-125, 202-202, 243-243
contracts/staking/src/tests/stake_tests.rs (1)
1-293: LGTM! Improved code formatting.The changes improve code readability through consistent formatting and proper indentation.
contracts/staking/src/tests/unstake_tests.rs (3)
204-206: LGTM: Configuration access aligned with new structure.The configuration access has been correctly updated to use the new nested structure, properly accessing the required fields from their respective config sections.
214-214: LGTM: Token denom access updated.The token denom is now correctly accessed from the protocol chain configuration.
405-421: LGTM: Configuration setup for receive_unstaked_tokens test.The test configuration setup has been properly updated to use the new nested configuration structure.
scripts/testnet/init-stake-contract.sh (2)
81-81: LGTM: Initialization message updated for new config structure.The initialization message correctly reflects the new nested configuration structure.
92-93: Verify oracle configuration update.The configuration update correctly includes the oracle address, but consider adding validation for the oracle contract's existence before updating.
manu0466
changed the title
Overview
This PR updates the
Configstructure used to store thestakingcontract configurations to improve future extensions.closes: LSTS-81, LSTS-82
Checklist