epic: new protobufs, revive nibi-stargate-perp, and cleanup #100
Conversation
…ssion abstraction
…smos bank, cosmos auth
Unique-Divine
added
type: bug/fix
WalkthroughThe changes encompass a broad restructuring and enhancement of the codebase, including updates to configuration files, WebAssembly artifacts, contract logic, error handling, and protobuf message definitions. The modifications aim to refine coverage settings, improve contract functionality, and align message handling with the Changes
Assessment against linked issues
Related issues
Poem
TipsChat with CodeRabbit Bot (
|
Codecov Report
Additional details and impacted files
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 16
Configuration used: CodeRabbit UI
Files ignored due to filter (18)
- Cargo.lock
- Cargo.toml
- artifacts/bindings_perp.wasm
- artifacts/cw3_flex_multisig.wasm
- artifacts/incentives.wasm
- artifacts/lockup.wasm
- artifacts/nibi_stargate.wasm
- artifacts/nibi_stargate_perp.wasm
- artifacts/nusd_valuator.wasm
- artifacts/pricefeed.wasm
- artifacts/shifter.wasm
- artifacts/token_vesting.wasm
- contracts/core-shifter/Cargo.toml
- contracts/nibi-stargate-perp/Cargo.toml
- contracts/nibi-stargate/Cargo.toml
- nibiru-std/Cargo.toml
- packages/core-controller/Cargo.lock
- packages/core-controller/Cargo.toml
Files selected for processing (39)
- .github/codecov.yml (1 hunks)
- artifacts/checksums.txt (1 hunks)
- artifacts/checksums_intermediate.txt (1 hunks)
- contracts/core-shifter/src/contract.rs (4 hunks)
- contracts/core-shifter/src/error.rs (1 hunks)
- contracts/core-shifter/src/lib.rs (1 hunks)
- contracts/core-shifter/src/msgs.rs (1 hunks)
- contracts/core-shifter/src/state.rs (1 hunks)
- contracts/core-shifter/src/testing.rs (1 hunks)
- contracts/nibi-stargate-perp/src/contract.rs (1 hunks)
- contracts/nibi-stargate-perp/src/integration_test.rs (1 hunks)
- contracts/nibi-stargate-perp/src/lib.rs (1 hunks)
- contracts/nibi-stargate-perp/src/msg.rs (1 hunks)
- contracts/nibi-stargate-perp/src/state.rs (1 hunks)
- contracts/nusd-valuator/.cargo/config (1 hunks)
- contracts/nusd-valuator/src/lib.rs (1 hunks)
- contracts/nusd-valuator/src/msgs.rs (3 hunks)
- contracts/nusd-valuator/src/queries.rs (1 hunks)
- contracts/nusd-valuator/src/testing.rs (1 hunks)
- nibiru-std/src/bindings/mod.rs (1 hunks)
- nibiru-std/src/bindings/msg.rs (2 hunks)
- nibiru-std/src/lib.rs (1 hunks)
- nibiru-std/src/proto/buf/cosmos.bank.v1beta1.rs (17 hunks)
- nibiru-std/src/proto/buf/cosmos.crypto.keyring.v1.rs (1 hunks)
- nibiru-std/src/proto/buf/cosmos.staking.v1beta1.rs (21 hunks)
- nibiru-std/src/proto/buf/nibiru.epochs.v1.rs (1 hunks)
- nibiru-std/src/proto/buf/nibiru.inflation.v1.rs (2 hunks)
- nibiru-std/src/proto/buf/nibiru.oracle.v1.rs (14 hunks)
- nibiru-std/src/proto/buf/nibiru.perp.v2.rs (8 hunks)
- nibiru-std/src/proto/buf/nibiru.spot.v1.rs (4 hunks)
- nibiru-std/src/proto/buf/nibiru.tokenfactory.v1.rs (7 hunks)
- nibiru-std/src/proto/mod.rs (3 hunks)
- nibiru-std/src/proto/type_url_cosmos.rs (2 hunks)
- nibiru-std/src/proto/type_url_nibiru.rs (6 hunks)
- nibiru-std/src/tutil/mod.rs (1 hunks)
- packages/core-controller/.cargo/config (1 hunks)
- packages/core-controller/src/contract.rs (9 hunks)
- packages/core-controller/src/msgs.rs (3 hunks)
- scripts/proto_clean.rs (6 hunks)
Files not summarized due to errors (2)
- nibiru-std/src/proto/buf/cosmos.staking.v1beta1.rs: Error: Message exceeds token limit
- nibiru-std/src/proto/buf/nibiru.perp.v2.rs: Error: Message exceeds token limit
Files not reviewed due to errors (1)
- nibiru-std/src/proto/buf/cosmos.staking.v1beta1.rs (Error: Message exceeds token limit)
Files skipped from review due to trivial changes (9)
- artifacts/checksums_intermediate.txt
- contracts/nusd-valuator/.cargo/config
- contracts/nusd-valuator/src/testing.rs
- nibiru-std/src/proto/buf/nibiru.oracle.v1.rs
- nibiru-std/src/proto/buf/nibiru.spot.v1.rs
- nibiru-std/src/proto/buf/nibiru.tokenfactory.v1.rs
- nibiru-std/src/proto/type_url_nibiru.rs
- nibiru-std/src/tutil/mod.rs
- packages/core-controller/.cargo/config
Additional comments: 74
.github/codecov.yml (1)
- 3-20: The changes to the
codecov.ymlfile reflect updates to the coverage range, precision, and how coverage status and comments are handled. These changes are consistent with the PR objectives to update configuration settings.artifacts/checksums.txt (1)
- 1-10: The checksums for the WebAssembly bytecode artifacts have been updated, which is consistent with the PR objectives of building new bytecode artifacts for updated contracts. However, the summary mentions the removal of
bindings_perp.wasm, which is not reflected in the provided hunk. Please verify ifbindings_perp.wasmhas been removed from the codebase.contracts/core-shifter/src/contract.rs (6)
1-6: The import changes align with the PR objectives and summary, removing outdated constructs and using updated modules.
20-33: The
instantiatefunction has been updated to set the contract version and initialize permissions, which is consistent with the PR objectives and summary.36-112: The
executefunction has been refactored to handle different message types, includingShiftSwapInvariant,ShiftPegMultiplier,EditOpers, andUpdateOwnership. This is in line with the PR objectives and summary.115-124: The
execute_update_ownershipfunction usescw_ownable::Action, which is part of thecw-ownablepackage mentioned in the PR objectives. This should have been highlighted in the summary.126-152: The
CanExecutestruct has been refactored to handle ownership and operator permissions, which is consistent with the PR objectives and summary.155-178: The
queryfunction has been updated to handle different query messages, which is consistent with the PR objectives and summary.contracts/core-shifter/src/error.rs (1)
- 1-15: The changes to the
ContractErrorenum are well-aligned with the PR objectives and follow Rust's idiomatic error handling practices.contracts/core-shifter/src/lib.rs (1)
- 1-6: The addition of the
errormodule is consistent with the PR objectives of improving error handling within thecore-shiftercontract. Thetestingmodule is correctly included only during testing, which is standard practice and should not affect production code.contracts/core-shifter/src/msgs.rs (5)
8-10: The renaming of the
adminfield toownerin theInitMsgstruct aligns with the PR objectives and the summary provided.15-24: The changes to the
ExecuteMsgenum, including the addition ofShiftSwapInvariant,ShiftPegMultiplier, andEditOpersvariants, are consistent with the PR objectives and the summary.27-34: The introduction of the
operator_permsmodule and theActionenum withAddOperandRemoveOpervariants is in line with the PR objectives and the summary.40-47: The updates to the
QueryMsgenum, including the addition ofHasPermsandPermsvariants, match the PR objectives and the summary.50-60: The
HasPermsResponseandPermsResponsestructs have been updated to align with the new permissions model, which is consistent with the PR objectives and the summary.contracts/core-shifter/src/state.rs (1)
- 59-127: The test cases have been appropriately updated to test the new functionality of the
Permissionsstruct. This ensures that the changes are verified and working as intended. It's good to see comprehensive testing around the new features.contracts/core-shifter/src/testing.rs (1)
- 17-33: The changes to the
setup_contractfunction align with the PR objectives of refactoring for cleaner code and improved maintainability.contracts/nibi-stargate-perp/src/contract.rs (4)
42-86: The
queryfunction is not implemented and contains atodo!()macro. Ensure that this is intentional and that the function will be implemented before the contract is used in a production environment.163-163: There are TODO comments within the
executefunction related to testing theExecuteMsg::MultiLiquidatevariant. Ensure that these tests are implemented before the contract is used in a production environment.180-180: There are TODO comments within the
executefunction related to testing theExecuteMsg::DonateToInsuranceFundvariant. Ensure that these tests are implemented before the contract is used in a production environment.280-295: The
query_contract_balancefunction correctly handles the case where thequeryresult isNoneby returning a defaultAllBalanceResponse. This is a safe and appropriate fallback.contracts/nibi-stargate-perp/src/integration_test.rs (2)
10-11: The TODO comment on line 10 suggests a test should be added to ensure the WASM file exists. Verify if this has been addressed and remove the comment if it is no longer relevant.
1-53: Ensure that the integration tests cover the new functionality mentioned in the PR objectives, such as the use of
CosmosMsg::Stargatefor message passing.contracts/nibi-stargate-perp/src/lib.rs (1)
- 1-3: The hunk shows the module structure of the
nibi-stargate-perpcontract. To ensure the PR objectives are met, the implementation details within these modules would need to be reviewed, particularly the use ofCosmosMsg::Stargatefor message passing as mentioned in the PR objectives and summary.contracts/nibi-stargate-perp/src/msg.rs (5)
6-10: The
InitMsgstruct is correctly defined with an optionaladminfield, aligning with the common pattern for smart contract instantiation messages.16-54: The
ExecuteMsgenum is well-defined, covering a range of contract operations such as market orders, position management, and insurance fund interactions. This aligns with the PR objectives of updating thenibi-stargate-perpcontract.56-60: The
LiquidationArgsstruct is appropriately defined for specifying arguments for liquidation operations.66-91: The
QueryMsgenum is well-structured, providing a clear interface for querying contract state, such as sudoers, markets, positions, module accounts, and oracle prices.93-96: The
SudoersQueryResponsestruct is correctly defined to encapsulate the response for a sudoers query.contracts/nibi-stargate-perp/src/state.rs (1)
- 1-94: The
Sudoersstruct and its methods, along with the associated tests, are correctly implemented and cover the required functionality. The use ofHashSetfor managing unique members and the comprehensive tests for theSudoersmethods, including storage operations, follow best practices and are well-structured.contracts/nusd-valuator/src/lib.rs (1)
- 13-14: The summary states that the
testingmodule has been moved outside of the conditional compilation directive#[cfg(test)], but the hunk shows that it is still within the directive. This discrepancy should be addressed to ensure accurate documentation.contracts/nusd-valuator/src/msgs.rs (3)
26-30: The summary states that
RedeemableChoiceshas been changed to returnBTreeSet<cw::Coin>instead ofBTreeSet<Coin>, but the hunk shows that it already returnsBTreeSet<cw::Coin>. Please verify if the summary is accurate or if the change was made in a previous commit.18-20: The summary indicates that the type of
to_denomin theRedeemablevariant was changed tocw::Uint128, but the hunk shows it is still aString. Please verify if the summary is accurate or if the change was made in a previous commit.12-13: The summary states that the type of
from_coinsin theMintablevariant was changed toBTreeSet<cw::Coin>, but the hunk shows it is stillBTreeSet<String>. Please verify if the summary is accurate or if the change was made in a previous commit.contracts/nusd-valuator/src/queries.rs (2)
68-71: The summary stating only the addition of a newline at the end of the file is incorrect. The hunk shows a logical change in the
query_redeemable_choicesfunction, which collects the results ofquery_redeemablecalls into a vector. This change should be reflected in the summary.68-71: The code in the hunk appears to be correct and does not exhibit any issues with the key areas of review.
nibiru-std/src/bindings/mod.rs (1)
- 7-9: The hunk shows the current state of the
bindings/mod.rsfile, which includes themsgmodule. There is no evidence in the hunk of the removal of thequerier,query, andstatemodules, but this is consistent with the PR objectives and the summary provided. No action is required based on this hunk.nibiru-std/src/bindings/msg.rs (1)
- 1-5: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-44]
The changes in
nibiru-std/src/bindings/msg.rsalign with the PR objectives and the provided summary. The removal of custom message handling and the simplification of message routing are reflected in the code. TheNibiruMsgenum is now used for message handling, and theLiquidationArgsstruct is correctly defined.nibiru-std/src/lib.rs (2)
9-9: The addition of the
tutilmodule should be documented in the PR summary or objectives if it introduces new functionality or changes existing behavior.12-12: The update of the
VERSION_NIBIRUconstant is in line with the PR objective of publishing a new version of thenibiru-stdcrate. Ensure that this version update is reflected wherever necessary across the codebase.nibiru-std/src/proto/buf/cosmos.bank.v1beta1.rs (1)
- 73-152: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [6-500]
The formatting changes to the
tagattributes in theprostmessage definitions are consistent and do not alter the functionality or logic of the code. These changes should not affect the serialization or deserialization of the protobuf messages as the tag numbers remain unchanged.nibiru-std/src/proto/buf/cosmos.crypto.keyring.v1.rs (1)
- 28-32: The update to the
pathfield in theLedgerstruct fromsuper::super::super::hd::v1::Bip44Paramstocrate::proto::cosmos::crypto::hd::v1::Bip44Paramsis consistent with the PR objectives and summary. Ensure that all references to this field are updated accordingly across the codebase to reflect the new module path.nibiru-std/src/proto/buf/nibiru.epochs.v1.rs (1)
- 54-60: The renaming of the structs
QueryEpochInfosRequestandQueryEpochInfosResponseis consistent with the summary provided and the PR objectives. Ensure that all references to these structs in the codebase are updated to reflect the new names.nibiru-std/src/proto/buf/nibiru.inflation.v1.rs (2)
7-35: The formatting changes in the
InflationDistributionandGenesisStatestructs are consistent with the PR objectives of cleaning up the codebase. These changes do not alter the functionality or the structure of the data.39-61: The addition of new fields
periods_per_yearandmax_periodto theParamsstruct aligns with the PR objectives of updating the codebase. Ensure that all relevant parts of the code that interact with theParamsstruct are updated to handle these new fields.nibiru-std/src/proto/buf/nibiru.perp.v2.rs (8)
4-58: The
Marketstruct definition appears to be correctly defined with appropriate field types and comments explaining each field.4-152: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [150-177]
The
Directionenum and its associated methods are correctly implemented and follow Rust's conventions for enums and associated functions.
- 215-250: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [215-271]
The
PositionChangedEventstruct definition appears to be correctly defined with appropriate field types and comments explaining each field.
276-341: The
PositionLiquidatedEventandLiquidationFailedEventstruct definitions appear to be correctly defined with appropriate field types and comments explaining each field.363-369: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [345-368]
The
LiquidationFailedReasonenum and its associated methods are correctly implemented and follow Rust's conventions for enums and associated functions.
388-399: The
AmmUpdatedEventstruct definition appears to be correctly defined with appropriate field types and comments explaining each field.409-769: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [409-945]
The
MarketUpdatedEventstruct and other related structs and messages for the genesis state and various events are correctly defined with appropriate field types and comments explaining each field.
- 772-981: The message structs such as
MsgMarketOrder,MsgMarketOrderResponse,MsgClosePosition,MsgClosePositionResponse,MsgPartialClose,MsgPartialCloseResponse, and others are correctly defined with appropriate field types and comments explaining each field.nibiru-std/src/proto/mod.rs (1)
- 79-223: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [28-106]
The addition of multiple sub-modules within the
cosmosmodule is consistent with the summary provided. This expansion aligns with the PR objectives of restructuring and expanding thenibiru-stdcrate's functionality.nibiru-std/src/proto/type_url_cosmos.rs (2)
6-32: The addition of constants for package names and implementations of the Name trait for various message types in the
cosmos.bank.v1beta1package aligns with the PR objectives and summary provided.53-157: The implementation of the Name trait for various message types in the
cosmos.auth.v1beta1andcosmos.gov.v1packages aligns with the PR objectives and summary provided.packages/core-controller/src/contract.rs (7)
1-7: The changes to the imports and module usage appear to be correct and there are no unused imports or missing dependencies.
139-145: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [135-145]
The implementation of
AddMembercorrectly adds a member to the whitelist and saves it.
151-157: The implementation of
RemoveMembercorrectly removes a member from the whitelist and saves it.166-172: The implementation of
ChangeAdmincorrectly changes the admin in the whitelist and saves it.315-324: The test case for adding a member correctly checks the response attributes and the state of the whitelist after the operation.
386-395: The test case for removing a member correctly checks the response attributes and the state of the whitelist after the operation.
450-459: The test case for changing the admin correctly checks the response attributes and the state of the whitelist after the operation.
packages/core-controller/src/msgs.rs (6)
1-1: The use of
#![allow(deprecated)]is consistent with the PR objectives to mark certain enum variants as deprecated.16-16: The deprecation note on
SetMarketEnabledis consistent with the PR objectives to deprecate certain functionalities in favor ofMsgServerimplementations.22-22: The deprecation note on
InsuranceFundWithdrawis consistent with the PR objectives to deprecate certain functionalities in favor ofMsgServerimplementations.26-26: The deprecation note on
EditOracleParamsis consistent with the PR objectives to deprecate certain functionalities in favor ofMsgServerimplementations.39-39: The deprecation note on
CreateMarketis consistent with the PR objectives to deprecate certain functionalities in favor ofMsgServerimplementations.13-29: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [13-42]
The changes to the
ExecuteMsgenum, including the addition of deprecation notes, align with the PR objectives and the summary provided.scripts/proto_clean.rs (5)
15-15: The addition of
PROTO_PATHaligns with the PR objectives to introduce updates and improvements.23-29: The updated file filtering logic to include "nibiru" prefixed files is consistent with the PR objectives.
106-122: The updates to the
super_import_to_cleanfunction reflect the PR objectives to modify conditions and prefixes for submodule imports.130-155: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [131-195]
The addition of functions and data structures in the
proto_submodulesmodule supports the modified import logic, aligning with the PR objectives.
- 224-241: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [199-241]
The new test cases added to the tests module are appropriate for validating the updated import logic, consistent with the PR objectives.
| // #[test] | ||
| // fn query_reserves() { | ||
| // let mut deps = mock_instance(WASM, &[]); | ||
| // let sender = String::from("sender"); | ||
| // let info = mock_info(&sender, &coins(1000, "unibi")); | ||
| // let inst_msg = InitMsg {}; | ||
| // let result: Response = | ||
| // instantiate(&mut deps, mock_env(), info, inst_msg).unwrap(); | ||
| // assert_eq!(result.messages.len(), 0); | ||
|
|
||
| // let pair = String::from("ueth:unusd"); | ||
| // let query_msg = NibiruQuery::Reserves { pair }; | ||
| // let raw_resp = query(&mut deps, mock_env(), query_msg); | ||
| // assert!(raw_resp.is_err(), "err: {}", raw_resp.unwrap_err()); | ||
| // let resp: ReservesResponse = | ||
| // from_slice(&raw_resp.unwrap(), DESERIALIZATION_LIMIT).unwrap(); | ||
| // assert_eq!(resp.pair, pair) | ||
| // } |
There was a problem hiding this comment.
The commented-out test code from lines 35-52 should be completed or removed if it's not intended to be used. Leaving commented-out code can lead to confusion and clutter in the codebase.
|
|
||
| pub const TEST_OWNER: &str = "owner"; | ||
|
|
||
| pub fn setup_contract(// accepted_denoms: Vec<String>, |
There was a problem hiding this comment.
The commented-out parameter // accepted_denoms: Vec<String>, should be removed if it's not used anywhere in the codebase.
- pub fn setup_contract(// accepted_denoms: Vec<String>,
+ pub fn setup_contract(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.
| pub fn setup_contract(// accepted_denoms: Vec<String>, | |
| pub fn setup_contract( |
| use std::collections::BTreeSet; | ||
|
|
||
| use cosmwasm_schema::cw_serde; | ||
| use cosmwasm_std::{Api, Storage}; | ||
| use cw_storage_plus::Item; | ||
|
|
||
| pub const WHITELIST: Item<Whitelist> = Item::new("whitelist"); | ||
| use crate::error::ContractError; | ||
|
|
||
| pub const OPERATORS: Item<BTreeSet<String>> = Item::new("operators"); | ||
|
|
||
| #[cw_serde] | ||
| pub struct Whitelist { | ||
| pub members: HashSet<String>, | ||
| pub admin: String, | ||
| pub struct Permissions { | ||
| pub owner: Option<String>, | ||
| pub operators: BTreeSet<String>, | ||
| } | ||
|
|
||
| impl Whitelist { | ||
| impl Permissions { | ||
| pub fn has(&self, addr: impl AsRef<str>) -> bool { | ||
| let addr = addr.as_ref(); | ||
| self.members.contains(addr) || self.admin == addr | ||
| self.operators.contains(addr) || self.is_owner(addr) | ||
| } | ||
|
|
||
| pub fn is_admin(&self, addr: impl AsRef<str>) -> bool { | ||
| pub fn is_owner(&self, addr: impl AsRef<str>) -> bool { | ||
| let addr = addr.as_ref(); | ||
| self.admin == addr | ||
| if let Some(owner) = &self.owner { | ||
| owner == addr | ||
| } else { | ||
| false | ||
| } | ||
| } | ||
|
|
||
| pub fn is_member(&self, addr: impl AsRef<str>) -> bool { | ||
| pub fn is_operator(&self, addr: impl AsRef<str>) -> bool { | ||
| let addr = addr.as_ref(); | ||
| self.members.contains(addr) | ||
| self.operators.contains(addr) | ||
| } | ||
|
|
||
| pub fn load(storage: &dyn Storage) -> Result<Self, ContractError> { | ||
| let owner = cw_ownable::get_ownership(storage)?.owner; | ||
| let opers = OPERATORS.load(storage)?; | ||
| Ok(Permissions { | ||
| owner: owner.map(|addr| addr.into_string()), | ||
| operators: opers, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| /// Set the given address as the contract owner and initialize the | ||
| /// 'OPERATORS' and 'OWNERSHIP' state. This function is only intended to be used only | ||
| /// during contract instantiation. | ||
| pub fn instantiate_perms( | ||
| owner: Option<&str>, | ||
| storage: &mut dyn Storage, | ||
| api: &dyn Api, | ||
| ) -> Result<(), ContractError> { | ||
| cw_ownable::initialize_owner(storage, api, owner)?; | ||
| Ok(OPERATORS.save(storage, &BTreeSet::default())?) | ||
| } |
There was a problem hiding this comment.
The changes to the Permissions struct and the addition of new functions such as is_owner, is_operator, load, and instantiate_perms align with the PR objectives of implementing a cleaner permissions abstraction using cw-ownable. The tests have been updated accordingly to reflect these changes. However, it's important to note that the switch from HashSet to BTreeSet for the operators field may have performance implications due to the ordered nature of BTreeSet, which should be considered if performance is a critical factor.
| @@ -81,36 +79,145 @@ pub mod cosmos { | |||
| } | |||
| } | |||
|
|
|||
| /// Defines base data structures like Coin, DecCoin, IntProto, and | |||
| /// DecProto. These types implement custo method signatures required by | |||
There was a problem hiding this comment.
Typographical error.
- /// DecProto. These types implement custo method signatures required by
+ /// DecProto. These types implement custom method signatures required byCommittable 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.
| /// DecProto. These types implement custo method signatures required by | |
| /// DecProto. These types implement custom method signatures required by |
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| pub mod tests { | ||
| use super::*; | ||
| use crate::{ | ||
| msgs::{ExecuteMsg, InitMsg}, | ||
| state::WHITELIST, | ||
| state::OPERATORS, | ||
| testing::{self as t, TestResult}, | ||
| }; | ||
|
|
||
| use cosmwasm_std::{coins, testing, Addr}; | ||
| use std::collections::HashSet; | ||
| use cosmwasm_std::{coins, testing}; | ||
| use std::collections::BTreeSet; | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Tests | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| #[test] | ||
| fn test_instantiate() { | ||
| fn test_instantiate() -> TestResult { | ||
| let mut deps = testing::mock_dependencies(); | ||
| let msg = InitMsg { | ||
| admin: "admin".to_string(), | ||
| owner: "admin".to_string(), | ||
| }; | ||
| let info: MessageInfo = | ||
| testing::mock_info("addr0000", &coins(2, "token")); | ||
|
|
||
| let result = | ||
| instantiate(deps.as_mut(), testing::mock_env(), info, msg).unwrap(); | ||
| let result = instantiate(deps.as_mut(), testing::mock_env(), info, msg)?; | ||
|
|
||
| assert_eq!(result.messages.len(), 0); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_has_admin_power() { | ||
| let admin = Addr::unchecked("admin"); | ||
| let msg = &InitMsg { | ||
| admin: admin.to_string(), | ||
| }; | ||
|
|
||
| fn test_has_admin_power() -> TestResult { | ||
| let sender = "not-admin"; | ||
| let mut deps = testing::mock_dependencies(); | ||
| let msg_info = testing::mock_info(sender, &coins(2, "token")); | ||
| instantiate(deps.as_mut(), testing::mock_env(), msg_info, msg.clone()) | ||
| .unwrap(); | ||
| let whitelist = WHITELIST.load(&deps.storage).unwrap(); | ||
| let has: bool = whitelist.is_admin(sender); | ||
| assert!(!has); | ||
|
|
||
| let sender = "admin"; | ||
| let mut deps = testing::mock_dependencies(); | ||
| let msg_info = testing::mock_info(sender, &coins(2, "token")); | ||
| instantiate(deps.as_mut(), testing::mock_env(), msg_info, msg.clone()) | ||
| .unwrap(); | ||
| let whitelist = WHITELIST.load(&deps.storage).unwrap(); | ||
| let has: bool = whitelist.is_admin(sender); | ||
| let (deps, _env, _info) = t::setup_contract()?; | ||
| let perms = Permissions::load(&deps.storage)?; | ||
| let not_has: bool = !perms.is_owner(sender); | ||
| assert!(not_has); | ||
| let sender = t::TEST_OWNER; | ||
| let has: bool = perms.is_owner(sender); | ||
| assert!(has); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_execute_unauthorized() { | ||
| let mut deps = testing::mock_dependencies(); | ||
| let admin = Addr::unchecked("admin"); | ||
|
|
||
| let msg = InitMsg { | ||
| admin: admin.as_str().to_string(), | ||
| }; | ||
| let msg_info = testing::mock_info("addr0000", &coins(2, "token")); | ||
| instantiate(deps.as_mut(), testing::mock_env(), msg_info, msg).unwrap(); | ||
|
|
||
| let execute_msg = ExecuteMsg::AddMember { | ||
| address: "addr0001".to_string(), | ||
| }; | ||
| fn test_exec_unauthorized() -> TestResult { | ||
| let (mut deps, _env, _info) = t::setup_contract()?; | ||
| let execute_msg = | ||
| ExecuteMsg::EditOpers(operator_perms::Action::AddOper { | ||
| address: "addr0001".to_string(), | ||
| }); | ||
| let unauthorized_info = testing::mock_info("unauthorized", &[]); | ||
| let result = execute( | ||
| deps.as_mut(), |
There was a problem hiding this comment.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [180-358]
The test cases have been updated to reflect the changes in the contract logic and functionality. However, the test_exec_change_admin function is marked as a TODO and does not contain any test logic. This should be implemented to ensure the execute_update_ownership function works as expected.
// TODO: Implement test logic for change owner functionality
#[test]
fn test_exec_change_admin() -> TestResult {
// Test logic should be added here
Ok(())
}| validator_fee_ratio, | ||
| } => { | ||
| check_member(check)?; | ||
| let cw_msg = ContractExecMsg { | ||
| route: NibiruRoute::Oracle, | ||
| msg: Some(ExecuteMsg::EditOracleParams { | ||
| vote_period, | ||
| vote_threshold, | ||
| reward_band, | ||
| whitelist, | ||
| slash_fraction, | ||
| slash_window, | ||
| min_valid_per_window, | ||
| twap_lookback_window, | ||
| min_voters, | ||
| validator_fee_ratio, | ||
| }), | ||
| todo!(); | ||
| let _cw_msg = ExecuteMsg::EditOracleParams { | ||
| vote_period, | ||
| vote_threshold, | ||
| reward_band, | ||
| whitelist, | ||
| slash_fraction, | ||
| slash_window, | ||
| min_valid_per_window, | ||
| twap_lookback_window, | ||
| min_voters, | ||
| validator_fee_ratio, | ||
| }; | ||
| let res = Response::new() | ||
| .add_message(cw_msg) | ||
| .add_attributes(vec![attr("action", "edit_oracle")]); | ||
| Ok(res) | ||
| // let res = Response::new() | ||
| // .add_message(cw_msg) | ||
| // .add_attributes(vec![attr("action", "edit_oracle")]); | ||
| // Ok(res) | ||
| } | ||
|
|
||
| ExecuteMsg::AddMember { address } => { |
There was a problem hiding this comment.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [104-130]
Incomplete functionality due to todo!() macro and commented-out code. This should be completed or removed before merging.
| #[entry_point] | ||
| pub fn instantiate( | ||
| deps: DepsMut, | ||
| _env: Env, | ||
| info: MessageInfo, | ||
| msg: InitMsg, | ||
| ) -> StdResult<Response> { | ||
| set_contract_version(deps.storage, CONTRACT_NAME, CONTRACT_VERSION)?; | ||
| let admin: String = match msg.admin { | ||
| Some(msg_admin) => msg_admin, | ||
| None => info.sender.to_string(), | ||
| }; | ||
| let sudoers = Sudoers { | ||
| members: vec![admin.clone()].into_iter().collect(), | ||
| admin, | ||
| }; | ||
| SUDOERS.save(deps.storage, &sudoers)?; | ||
| Ok(Response::new() | ||
| .add_attribute("action", "instantiate") | ||
| .add_attribute("owner", info.sender)) | ||
| } |
There was a problem hiding this comment.
The instantiate function should return Result<Response, ContractError> as per the PR objectives, but it currently returns StdResult<Response>. This needs to be updated to align with the new error handling strategy.
| can_execute.check_member()?; | ||
| let cosmos_msg: CosmosMsg = nibiru::perp::MsgAddMargin { | ||
| sender: contract_addr, | ||
| pair, | ||
| margin: Some(margin.into()), | ||
| } | ||
| .into_stargate_msg(); | ||
| Ok(Response::new().add_message(cosmos_msg)) | ||
| } | ||
|
|
||
| ExecuteMsg::RemoveMargin { pair, margin } => { | ||
| can_execute.check_member()?; | ||
| let cosmos_msg: CosmosMsg = nibiru::perp::MsgRemoveMargin { | ||
| sender: contract_addr, | ||
| pair, | ||
| margin: Some(margin.into()), | ||
| } | ||
| .into_stargate_msg(); | ||
| Ok(Response::new().add_message(cosmos_msg)) | ||
| } | ||
|
|
||
| // TODO:test ExecuteMsg::MultiLiquidate | ||
| ExecuteMsg::MultiLiquidate { liquidations } => { | ||
| can_execute.check_member()?; | ||
| let cosmos_msg: CosmosMsg = nibiru::perp::MsgMultiLiquidate { | ||
| sender: contract_addr, | ||
| liquidations: liquidations | ||
| .iter() | ||
| .map(|la| nibiru::perp::msg_multi_liquidate::Liquidation { | ||
| pair: la.pair.clone(), | ||
| trader: la.trader.clone(), | ||
| }) | ||
| .collect(), | ||
| } | ||
| .into_stargate_msg(); | ||
| Ok(Response::new().add_message(cosmos_msg)) | ||
| } | ||
|
|
||
| // TODO:test ExecuteMsg::DonateToInsuranceFund | ||
| ExecuteMsg::DonateToInsuranceFund { donation } => { | ||
| can_execute.check_member()?; | ||
| let cosmos_msg: CosmosMsg = nibiru::perp::MsgDonateToEcosystemFund { | ||
| sender: contract_addr, | ||
| donation: Some(donation.into()), | ||
| } | ||
| .into_stargate_msg(); | ||
| Ok(Response::new().add_message(cosmos_msg)) | ||
| } | ||
|
|
||
| ExecuteMsg::Claim { | ||
| funds, | ||
| claim_all, | ||
| to, | ||
| } => { | ||
| can_execute.check_admin()?; | ||
| let event_key = "execute_claim"; | ||
| if let Some(claim_all_value) = claim_all { | ||
| if !claim_all_value { | ||
| return Err(StdError::generic_err( | ||
| "setting 'claim_all' to false causes an error: " | ||
| .to_string() | ||
| + "try removing claim_all as an argument entirely.", | ||
| )); | ||
| } | ||
| let contract_address = env_ctx.contract.address; | ||
| let balances = query_contract_balance( | ||
| contract_address.to_string(), | ||
| deps.as_ref(), | ||
| )?; | ||
|
|
||
| // Send all funds to the specified recipient. | ||
| let transfer_msg = BankMsg::Send { | ||
| to_address: to.clone(), | ||
| amount: balances.amount, | ||
| }; | ||
| Ok(Response::new().add_message(transfer_msg).add_attribute( | ||
| event_key, | ||
| format!("successfully claimed to {}", to), | ||
| )) | ||
| } else if let Some(funds_value) = funds { | ||
| // Send all funds to the specified recipient. | ||
| let transfer_msg = BankMsg::Send { | ||
| to_address: to.clone(), | ||
| amount: vec![funds_value], | ||
| }; | ||
| return Ok(Response::new() | ||
| .add_message(transfer_msg) | ||
| .add_attribute( | ||
| event_key, | ||
| format!("successfully claimed to {}", to), | ||
| )); | ||
| } else { | ||
| return Err(StdError::generic_err( | ||
| "either the 'funds' or 'claim_all' arguments must be specified")); | ||
| } | ||
| } // TODO test: add member | ||
| // TODO test: remove member | ||
| // TODO test: change admin | ||
| } | ||
| } |
There was a problem hiding this comment.
The execute function should return Result<Response, ContractError> as per the PR objectives, but it currently returns StdResult<Response>. This needs to be updated to align with the new error handling strategy.
| ExecuteMsg::Claim { | ||
| funds, | ||
| claim_all, | ||
| to, | ||
| } => { | ||
| can_execute.check_admin()?; | ||
| let event_key = "execute_claim"; | ||
| if let Some(claim_all_value) = claim_all { | ||
| if !claim_all_value { | ||
| return Err(StdError::generic_err( | ||
| "setting 'claim_all' to false causes an error: " | ||
| .to_string() | ||
| + "try removing claim_all as an argument entirely.", | ||
| )); | ||
| } | ||
| let contract_address = env_ctx.contract.address; | ||
| let balances = query_contract_balance( | ||
| contract_address.to_string(), | ||
| deps.as_ref(), | ||
| )?; | ||
|
|
||
| // Send all funds to the specified recipient. | ||
| let transfer_msg = BankMsg::Send { | ||
| to_address: to.clone(), | ||
| amount: balances.amount, | ||
| }; | ||
| Ok(Response::new().add_message(transfer_msg).add_attribute( | ||
| event_key, | ||
| format!("successfully claimed to {}", to), | ||
| )) | ||
| } else if let Some(funds_value) = funds { | ||
| // Send all funds to the specified recipient. | ||
| let transfer_msg = BankMsg::Send { | ||
| to_address: to.clone(), | ||
| amount: vec![funds_value], | ||
| }; | ||
| return Ok(Response::new() | ||
| .add_message(transfer_msg) | ||
| .add_attribute( | ||
| event_key, | ||
| format!("successfully claimed to {}", to), | ||
| )); | ||
| } else { | ||
| return Err(StdError::generic_err( | ||
| "either the 'funds' or 'claim_all' arguments must be specified")); | ||
| } | ||
| } // TODO test: add member |
There was a problem hiding this comment.
The ExecuteMsg::Claim variant in the execute function contains a condition that errors out if claim_all is set to false. This seems unnecessary as the claim_all field is a bool and should be able to handle both true and false values. Consider revising this logic to allow for both possibilities.
| fn check_can_execute(deps: Deps, sender: &str) -> StdResult<CanExecute> { | ||
| let sudoers = SUDOERS.load(deps.storage).unwrap(); | ||
| Ok(CanExecute { | ||
| is_admin: sudoers.is_admin(sender), | ||
| is_member: sudoers.is_member(sender), | ||
| sender: sender.into(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
The check_can_execute function unwraps the result of SUDOERS.load without handling the potential error. This could lead to a panic if the load fails. It's better to handle the error and return a ContractError if the load is unsuccessful.
Context
core-shiftersmart contract now usescw-ownableand has a cleaner permissions abstraction.CosmosMsg::StargateMsgnibiru#1580NibiruRouteenum was removed everywhere.nibiru-stdv0.0.3Commit Log
CosmosMsg::StargateMsgnibiru#1580Summary by CodeRabbit
Documentation
ContractErrortype.Refactor
cosmospackage.Style
prostmessage definitions by adjusting thetagattribute formatting.Tests
Chores
Bug Fixes