feat(broker-bank): smart contract as broker for bank transfers #134
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe updates introduce a comprehensive suite of functionalities for a broker-bank smart contract, focusing on operational permissions, error handling, event management, and state management. Key features include initializing contracts, managing bank sends, toggling operations, updating ownership, editing operators, withdrawing funds, and handling permissions. Additionally, there's a reorganization in the core-shifter contract and a minor update in the standard library to support testing. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
|
Unique-Divine
commented
Mar 8, 2024
nibiru-std/src/errors.rs
Outdated
There was a problem hiding this comment.
Removing #[cfg(test)] is necessary for external crates to import this type.
Unique-Divine
force-pushed
the
ud/broker-bank
branch
from
40493f9 to
aa32d83
Compare
2 tasks
![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: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
Cargo.lockis excluded by:!**/*.lockcontracts/broker-bank/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (13)
- .gitignore (1 hunks)
- contracts/broker-bank/.cargo/config (1 hunks)
- contracts/broker-bank/src/contract.rs (1 hunks)
- contracts/broker-bank/src/error.rs (1 hunks)
- contracts/broker-bank/src/events.rs (1 hunks)
- contracts/broker-bank/src/lib.rs (1 hunks)
- contracts/broker-bank/src/msgs.rs (1 hunks)
- contracts/broker-bank/src/oper_perms.rs (1 hunks)
- contracts/broker-bank/src/state.rs (1 hunks)
- contracts/broker-bank/src/tutil.rs (1 hunks)
- contracts/core-shifter/src/contract.rs (5 hunks)
- contracts/core-shifter/src/msgs.rs (3 hunks)
- nibiru-std/src/errors.rs (1 hunks)
Files skipped from review due to trivial changes (2)
- .gitignore
- contracts/broker-bank/.cargo/config
Additional comments: 31
contracts/broker-bank/src/lib.rs (2)
- 4-5: The conditional compilation for the
tutilmodule ensures it's only included during testing, which is a good practice to avoid including test utilities in the production build.- 7-11: The conditional compilation to exclude
contract.rswhen the "library" feature is enabled is a smart way to prevent entry point errors and make the contract usable as a library. This approach enhances modularity and reusability.contracts/broker-bank/src/events.rs (4)
- 5-9: The
event_bank_sendfunction correctly creates an event for bank sends with appropriate attributes. This follows best practices for event creation in smart contracts, ensuring traceability and transparency of actions.- 11-14: The
event_toggle_haltfunction properly creates an event for toggling the halt status of operations. Using the boolean status converted to a string as an attribute is a clear way to indicate the new state.- 16-20: The
event_withdrawfunction is well-implemented for creating withdrawal events, including the necessary attributes for tracking the operation. This enhances the auditability of transactions.- 22-26: The
denom_set_jsonfunction provides a utility for serializing a set of denominations to JSON. Leveragingserde_jsonfor this task is efficient and reduces the potential for serialization errors.contracts/broker-bank/src/state.rs (5)
- 6-8: The
TO_ADDRSconstant is correctly defined usingItemfor a set of addresses. This is a good practice for managing a whitelist of recipient addresses in the contract state.- 10-12: The
OPERATORSconstant usesItemto store a set of operator addresses. This approach is efficient for managing permissions within the smart contract.- 14-17: The
LOGSconstant is well-defined usingDequeto maintain a queue of log entries. This is suitable for tracking transaction and event logs over time, allowing for efficient append and retrieval operations.- 19-20: The
IS_HALTEDconstant usesItemto store a boolean flag indicating whether operations are currently halted. This is a straightforward and effective way to manage operational status.- 22-29: The
Logstruct is appropriately designed to record execute transaction logs, including block height, sender address, and the event. Usingcw_serdefor serialization ensures compatibility with the CosmWasm framework.contracts/broker-bank/src/error.rs (2)
- 6-39: The
ContractErrorenum comprehensively covers various error scenarios, including standard errors, JSON serialization errors, ownership errors, and specific contract operation errors. This approach ensures robust error handling throughout the contract.- 41-44: The implementation of
From<serde_json::Error>forContractErroris correctly done, allowing for seamless conversion of JSON serialization errors into contract errors. This enhances error handling consistency.nibiru-std/src/errors.rs (1)
- 3-3: Removing the conditional compilation attribute from
TestResultmakes it universally available, simplifying error handling in tests. This change is beneficial for consistency across the codebase.contracts/core-shifter/src/msgs.rs (1)
- 26-45: Moving
HasPermsResponseandPermsResponsestructs into theoperator_permssubmodule and updatingQueryMsgvariants to return types from this module is a good practice for organizing related functionalities. This enhances code maintainability and readability.contracts/broker-bank/src/tutil.rs (3)
- 16-35: The
setup_contractfunction provides a flexible way to initialize the contract with custom addresses and operators for testing. This is a valuable utility for creating diverse test scenarios.- 38-51: The
setup_contract_defaultsfunction simplifies the process of setting up the contract with default parameters for tests. This utility function is helpful for quickly initializing common test setups.- 54-61: The utility functions
mock_info_for_senderandmock_env_heightare well-implemented for creating mockMessageInfoandEnvwith specific sender and block height. These functions are essential for testing contract behavior under different conditions.contracts/broker-bank/src/msgs.rs (3)
- 8-33: The
ExecuteMsgenum is well-structured, covering essential contract operations such as bank sends, toggling halt status, withdrawals, and editing operators. This comprehensive coverage ensures that the contract can be interacted with effectively through these messages.- 35-44: The
QueryMsgenum, including thePermsquery, is correctly defined to allow querying the contract's permissions status. This is crucial for external entities to understand the operational status and permissions setup of the contract.- 46-58: The
InstantiateMsgstruct is appropriately designed to capture the necessary information for initializing the contract, including the owner, whitelisted addresses, and operators. This ensures that the contract is set up with clear ownership and operational permissions from the start.contracts/broker-bank/src/oper_perms.rs (3)
- 8-12: The
Actionenum provides a clear and concise way to represent actions related to operator permissions, such as adding or removing an operator. This is an effective approach to encapsulating permission-related actions.- 14-24: The response structs
HasPermsResponseandPermsResponseare well-defined, offering a structured way to respond to permission queries. This ensures that permission-related information is communicated clearly and effectively.- 26-70: The
Permissionsstruct and its associated functions, including loading permissions from storage and asserting operator status, are correctly implemented. These functionalities are essential for managing and validating operator permissions within the contract.contracts/core-shifter/src/contract.rs (1)
- 164-170: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [15-176]
The updates to the contract's entry points and permission management logic, including the reorganization of permission-related message structures into a submodule, are correctly implemented. These changes improve the contract's structure and maintainability.
contracts/broker-bank/src/contract.rs (6)
- 25-41: The instantiation logic correctly sets up the contract version, initializes the owner, and sets initial state values. However, consider validating the
to_addrsandopersinInstantiateMsgto ensure they meet any specific criteria (e.g., address format) before saving them to state. This can prevent potential issues with invalid data being stored.- 18-23: The
ContractErrorenum is well-defined and covers a broad range of potential error scenarios. Ensure that all functions that can fail are using this enum for error handling, and consider adding more specific errors if any scenarios are not adequately covered.- 19-20: Event management through
event_bank_send,event_toggle_halt, andevent_withdrawfunctions is implemented correctly. Ensure that all significant contract actions are accompanied by event logging for transparency and auditability.- 13-22: State management is handled appropriately with the use of the
TO_ADDRS,OPERATORS, andIS_HALTEDstorage items. Consider adding checks or validations when updating these state items to ensure that only valid data is stored. For example, when adding operators, ensure that the addresses are valid blockchain addresses.- 212-227: The query functionalities for permissions status and ownership are implemented correctly. However, ensure that the data returned by these queries does not expose sensitive information or compromise the contract's security. Additionally, consider caching frequently accessed data to improve query performance.
- 275-698: The tests provided cover a range of functionalities and scenarios, which is excellent for ensuring contract reliability. A few suggestions for improvement:
- Increase coverage for edge cases and error conditions to ensure robustness.
- Consider using parameterized tests to reduce code duplication and cover more scenarios with less code.
- Ensure that all public and critical internal functions are covered by tests.
Comment on lines
+44
to
+188
| cw_ownable::assert_owner(deps.storage, &info.sender)?; | ||
| let new_is_halted = !IS_HALTED.load(deps.storage)?; | ||
| IS_HALTED.save(deps.storage, &new_is_halted)?; | ||
| Ok(Response::new().add_event(event_toggle_halt(&new_is_halted))) | ||
| } | ||
|
|
||
| ExecuteMsg::UpdateOwnership(action) => { | ||
| Ok(execute_update_ownership(deps, env, info, action)?) | ||
| } | ||
|
|
||
| ExecuteMsg::EditOpers(action) => { | ||
| cw_ownable::assert_owner(deps.storage, &info.sender)?; | ||
| let mut perms = Permissions::load(deps.storage)?; | ||
| let api = deps.api; | ||
| match action { | ||
| oper_perms::Action::AddOper { address } => { | ||
| let addr = api.addr_validate(address.as_str())?; | ||
| perms.operators.insert(addr.into_string()); | ||
| OPERATORS.save(deps.storage, &perms.operators)?; | ||
|
|
||
| let res = Response::new().add_attributes(vec![ | ||
| attr("action", "add_operator"), | ||
| attr("address", address), | ||
| ]); | ||
| Ok(res) | ||
| } | ||
|
|
||
| oper_perms::Action::RemoveOper { address } => { | ||
| perms.operators.remove(address.as_str()); | ||
| OPERATORS.save(deps.storage, &perms.operators)?; | ||
|
|
||
| let res = Response::new().add_attributes(vec![ | ||
| attr("action", "remove_operator"), | ||
| attr("address", address), | ||
| ]); | ||
| Ok(res) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| ExecuteMsg::WithdrawAll { to } => { | ||
| cw_ownable::assert_owner(deps.storage, &info.sender)?; | ||
| let to_addr: String = match to { | ||
| Some(given_to_addr) => given_to_addr, | ||
| None => info.sender.to_string(), | ||
| }; | ||
| let balances = query_bank_balances(contract_addr, deps.as_ref())?; | ||
| let tx_msg = BankMsg::Send { | ||
| to_address: to_addr.to_string(), | ||
| amount: balances.amount.clone(), | ||
| }; | ||
| let event = event_withdraw( | ||
| serde_json::to_string(&balances.amount)?.as_str(), | ||
| &to_addr, | ||
| ); | ||
| LOGS.push_front( | ||
| deps.storage, | ||
| &Log { | ||
| block_height: env.block.height, | ||
| sender_addr: info.sender.to_string(), | ||
| event: event.clone(), | ||
| }, | ||
| )?; | ||
| Ok(Response::new().add_message(tx_msg).add_event(event)) | ||
| } | ||
|
|
||
| ExecuteMsg::Withdraw { to, denoms } => { | ||
| cw_ownable::assert_owner(deps.storage, &info.sender)?; | ||
| let to_addr: String = match to { | ||
| Some(given_to_addr) => given_to_addr, | ||
| None => info.sender.to_string(), | ||
| }; | ||
| let balances: AllBalanceResponse = | ||
| query_bank_balances(contract_addr, deps.as_ref())?; | ||
| let balances: Vec<cw_std::Coin> = balances | ||
| .amount | ||
| .iter() | ||
| .filter(|b_coin| denoms.contains(&b_coin.denom)) | ||
| .cloned() | ||
| .collect(); | ||
|
|
||
| let tx_msg = BankMsg::Send { | ||
| to_address: to_addr.to_string(), | ||
| amount: balances.clone(), | ||
| }; | ||
| let event = event_withdraw( | ||
| serde_json::to_string(&balances)?.as_str(), | ||
| &to_addr, | ||
| ); | ||
| LOGS.push_front( | ||
| deps.storage, | ||
| &Log { | ||
| block_height: env.block.height, | ||
| sender_addr: info.sender.to_string(), | ||
| event: event.clone(), | ||
| }, | ||
| )?; | ||
| Ok(Response::new().add_message(tx_msg).add_event(event)) | ||
| } | ||
| } |
There was a problem hiding this comment.
The execution logic is well-structured and covers a range of functionalities. A few points to consider:
- In the
BankSendcase, ensure that thecoinsandtofields in theExecuteMsgare validated before proceeding with the transaction. - The
assert_not_haltedfunction's logic seems inverted; it should returnOk(())whenis_haltedisfalse, nottrue. This is critical for the correct functioning of the contract. - For
EditOpers, consider adding logging for the addition and removal of operators for better auditability.
- true => Ok(()),
+ false => Ok(()),
- false => Err(ContractError::OperationsHalted),
+ true => Err(ContractError::OperationsHalted),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.
Suggested change
| #[cfg_attr(not(feature = "library"), cosmwasm_std::entry_point)] | |
| pub fn execute( | |
| deps: DepsMut, | |
| env: Env, | |
| info: MessageInfo, | |
| msg: ExecuteMsg, | |
| ) -> Result<Response, ContractError> { | |
| let contract_addr = env.contract.address.to_string(); | |
| match msg { | |
| ExecuteMsg::BankSend { coins, to } => { | |
| // assert sender is operator | |
| Permissions::assert_operator(deps.storage, info.sender.to_string())?; | |
| // assert: Operator execute calls should not be halted. | |
| let is_halted = IS_HALTED.load(deps.storage)?; | |
| assert_not_halted(is_halted)?; | |
| // assert: Recipient addr must be in the TO_ADDRS set. | |
| if !TO_ADDRS.load(deps.storage)?.contains(&to) { | |
| return Err(ContractError::ToAddrNotAllowed { | |
| to_addr: to.to_string(), | |
| }); | |
| } | |
| // Events and tx history logging | |
| let coins_json = serde_json::to_string(&coins)?; | |
| let event = event_bank_send(&coins_json, info.sender.as_str()); | |
| LOGS.push_front( | |
| deps.storage, | |
| &Log { | |
| block_height: env.block.height, | |
| sender_addr: info.sender.to_string(), | |
| event: event.clone(), | |
| }, | |
| )?; | |
| // Reply with TxMsg to send funds | |
| Ok(Response::new() | |
| .add_message(BankMsg::Send { | |
| to_address: to, | |
| amount: coins, | |
| }) | |
| .add_event(event)) | |
| } | |
| ExecuteMsg::ToggleHalt {} => { | |
| cw_ownable::assert_owner(deps.storage, &info.sender)?; | |
| let new_is_halted = !IS_HALTED.load(deps.storage)?; | |
| IS_HALTED.save(deps.storage, &new_is_halted)?; | |
| Ok(Response::new().add_event(event_toggle_halt(&new_is_halted))) | |
| } | |
| ExecuteMsg::UpdateOwnership(action) => { | |
| Ok(execute_update_ownership(deps, env, info, action)?) | |
| } | |
| ExecuteMsg::EditOpers(action) => { | |
| cw_ownable::assert_owner(deps.storage, &info.sender)?; | |
| let mut perms = Permissions::load(deps.storage)?; | |
| let api = deps.api; | |
| match action { | |
| oper_perms::Action::AddOper { address } => { | |
| let addr = api.addr_validate(address.as_str())?; | |
| perms.operators.insert(addr.into_string()); | |
| OPERATORS.save(deps.storage, &perms.operators)?; | |
| let res = Response::new().add_attributes(vec![ | |
| attr("action", "add_operator"), | |
| attr("address", address), | |
| ]); | |
| Ok(res) | |
| } | |
| oper_perms::Action::RemoveOper { address } => { | |
| perms.operators.remove(address.as_str()); | |
| OPERATORS.save(deps.storage, &perms.operators)?; | |
| let res = Response::new().add_attributes(vec![ | |
| attr("action", "remove_operator"), | |
| attr("address", address), | |
| ]); | |
| Ok(res) | |
| } | |
| } | |
| } | |
| ExecuteMsg::WithdrawAll { to } => { | |
| cw_ownable::assert_owner(deps.storage, &info.sender)?; | |
| let to_addr: String = match to { | |
| Some(given_to_addr) => given_to_addr, | |
| None => info.sender.to_string(), | |
| }; | |
| let balances = query_bank_balances(contract_addr, deps.as_ref())?; | |
| let tx_msg = BankMsg::Send { | |
| to_address: to_addr.to_string(), | |
| amount: balances.amount.clone(), | |
| }; | |
| let event = event_withdraw( | |
| serde_json::to_string(&balances.amount)?.as_str(), | |
| &to_addr, | |
| ); | |
| LOGS.push_front( | |
| deps.storage, | |
| &Log { | |
| block_height: env.block.height, | |
| sender_addr: info.sender.to_string(), | |
| event: event.clone(), | |
| }, | |
| )?; | |
| Ok(Response::new().add_message(tx_msg).add_event(event)) | |
| } | |
| ExecuteMsg::Withdraw { to, denoms } => { | |
| cw_ownable::assert_owner(deps.storage, &info.sender)?; | |
| let to_addr: String = match to { | |
| Some(given_to_addr) => given_to_addr, | |
| None => info.sender.to_string(), | |
| }; | |
| let balances: AllBalanceResponse = | |
| query_bank_balances(contract_addr, deps.as_ref())?; | |
| let balances: Vec<cw_std::Coin> = balances | |
| .amount | |
| .iter() | |
| .filter(|b_coin| denoms.contains(&b_coin.denom)) | |
| .cloned() | |
| .collect(); | |
| let tx_msg = BankMsg::Send { | |
| to_address: to_addr.to_string(), | |
| amount: balances.clone(), | |
| }; | |
| let event = event_withdraw( | |
| serde_json::to_string(&balances)?.as_str(), | |
| &to_addr, | |
| ); | |
| LOGS.push_front( | |
| deps.storage, | |
| &Log { | |
| block_height: env.block.height, | |
| sender_addr: info.sender.to_string(), | |
| event: event.clone(), | |
| }, | |
| )?; | |
| Ok(Response::new().add_message(tx_msg).add_event(event)) | |
| } | |
| } | |
| true => Err(ContractError::OperationsHalted), | |
| false => Ok(()), |
Comment on lines
+28
to
+33
| pub fn event_migrate(_arg0: &u64, _arg1: &bool) -> Event { | ||
| // Event::new("migrate_nusd_valuator") | ||
| // .add_attribute("id", _arg0.to_string()) | ||
| // .add_attribute("new_done", _arg1.to_string()) | ||
| todo!(); // TODO: event migrate | ||
| } |
There was a problem hiding this comment.
The event_migrate function is currently a placeholder marked with todo!(). This indicates unfinished functionality that should be addressed before finalizing the contract.
Would you like assistance in implementing the migration event logic, or should this be tracked as a separate task?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- contracts/broker-bank/src/contract.rs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- contracts/broker-bank/src/contract.rs
matthiasmatt
approved these changes
Mar 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Abstract / Context
🚧 (Still draft, but you can review the main logic now). 🚧
Tasks for Completion
Commit Log
Summary by CodeRabbit
New Features
Refactor
Chores
.gitignoreto exclude specific schema directories.TestResulttype alias available for general use by removing conditional compilation attribute.