Prevent from sending tokens when calling a non-payable function on Od… #459
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant ContractContainer
participant EntryPoint
participant VM
User->>ContractContainer: call(CallDef)
ContractContainer->>EntryPoint: check is_payable and amount
alt is_payable and amount > 0
ContractContainer->>User: ExecutionError::NonPayable
else
ContractContainer->>VM: execute call
VM->>ContractContainer: return result
ContractContainer->>User: return result
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- core/src/contract_container.rs (2 hunks)
- core/src/entry_point_callback.rs (1 hunks)
- examples/src/features/native_token.rs (1 hunks)
- odra-macros/src/ast/deployer_item.rs (2 hunks)
- odra-macros/src/ast/deployer_utils.rs (1 hunks)
- odra-macros/src/ast/test_parts.rs (2 hunks)
- odra-macros/src/utils/expr.rs (1 hunks)
Additional comments not posted (8)
examples/src/features/native_token.rs (1)
51-63: The new test functiontest_call_non_payable_function_with_tokenscorrectly simulates the scenario of calling a non-payable function with tokens. Consider adding more assertions to cover different edge cases or error messages for comprehensive testing.core/src/entry_point_callback.rs (1)
68-89: The addition of theis_payablefield and the new constructor methodsnewandnew_payablein theEntryPointstruct are well-implemented. Consider adding detailed documentation for theis_payablefield and the behavior of these methods to enhance code readability and maintainability.core/src/contract_container.rs (1)
26-36: The modifications in thecallmethod to handle non-payable entry points when tokens are sent are correctly implemented. Consider adding unit tests to cover scenarios where tokens are mistakenly sent to non-payable functions to ensure robust error handling.odra-macros/src/utils/expr.rs (1)
167-179: The update to thenew_entry_pointfunction to support both payable and non-payable entry points is well-implemented. Consider adding comprehensive tests for this function to ensure that it correctly handles both types of entry points under various scenarios.odra-macros/src/ast/deployer_utils.rs (1)
24-27: The changes to include theis_payableattribute in theEntrypointsInitExprstruct are correctly implemented. Consider adding comments or documentation explaining the use and impact of theis_payableattribute to enhance code clarity.odra-macros/src/ast/deployer_item.rs (1)
Line range hint
217-277: The changes to theDeployerItemstruct to support the creation of payable entry points are well-implemented. Consider adding tests to ensure that the deployment process correctly handles payable entry points under various scenarios.odra-macros/src/ast/test_parts.rs (2)
295-295: Update toEntryPoint::new_payablereflects the new "payable" functionality correctly.This change correctly updates the entry point for "pay_to_mint" to be payable, aligning with the new system requirements for handling token transactions. Good job ensuring that the system's behavior matches the intended functionality.
457-457: Correct implementation of the payable entry point for "pay_to_mint".This change is consistent with the updates made elsewhere in the codebase to support the new "payable" functionality. It ensures that the "pay_to_mint" function can correctly handle token transactions, which is crucial for the correct operation of the contract.
* Update proxy_caller and OdraVm * proxy caller ensures if the cargo purse is empty * update OdraVm's call_contract to revert on error
Benchmark report
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
odra-casper/test-vm/resources/proxy_caller.wasmis excluded by!**/*.wasmodra-casper/test-vm/resources/proxy_caller_with_return.wasmis excluded by!**/*.wasm
Files selected for processing (5)
- examples/src/features/native_token.rs (1 hunks)
- odra-casper/proxy-caller/bin/proxy_caller.rs (1 hunks)
- odra-casper/proxy-caller/bin/proxy_caller_with_return.rs (2 hunks)
- odra-casper/proxy-caller/src/lib.rs (1 hunks)
- odra-vm/src/vm/odra_vm.rs (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- examples/src/features/native_token.rs
Additional comments not posted (8)
odra-casper/proxy-caller/bin/proxy_caller.rs (2)
10-12: Ensure correct usage of new imports and function call.The addition of
ensure_cargo_purse_is_emptyaligns with the PR's goal to prevent token transfer when calling non-payable functions. Ensure that the function is tested thoroughly.
23-23: Verify the placement and logic ofensure_cargo_purse_is_empty.Verification successful
The function
ensure_cargo_purse_is_emptyis designed to ensure that the cargo purse is empty and reverts if it is not. It is used in two places:proxy_caller_with_return.rsandproxy_caller.rs. In both files, it is called after the proxy call is made, which seems appropriate as it ensures the cargo purse is empty after the operation.
- In
proxy_caller_with_return.rs, it is called aftercall_versioned_contract_ret_bytes.- In
proxy_caller.rs, it is called afterruntime::call_versioned_contract.Both usages appear to be correctly placed to ensure the cargo purse is empty after the respective operations.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `ensure_cargo_purse_is_empty` is called correctly and only when necessary. # Test: Search for the function usage. Expect: Only occurances where it's necessary. rg --type rust $'ensure_cargo_purse_is_empty'Length of output: 613
Script:
#!/bin/bash # Extract the definition of `ensure_cargo_purse_is_empty` rg --type rust --context 10 'pub fn ensure_cargo_purse_is_empty' odra-casper/proxy-caller/src/lib.rs # Extract the surrounding code where `ensure_cargo_purse_is_empty` is used in `proxy_caller_with_return.rs` rg --type rust --context 10 'ensure_cargo_purse_is_empty' odra-casper/proxy-caller/bin/proxy_caller_with_return.rs # Extract the surrounding code where `ensure_cargo_purse_is_empty` is used in `proxy_caller.rs` rg --type rust --context 10 'ensure_cargo_purse_is_empty' odra-casper/proxy-caller/bin/proxy_caller.rsLength of output: 2460
odra-casper/proxy-caller/bin/proxy_caller_with_return.rs (2)
10-12: Ensure correct usage of new imports and function call.The addition of
ensure_cargo_purse_is_emptyaligns with the PR's goal to prevent token transfer when calling non-payable functions. Ensure that the function is tested thoroughly.
25-25: Verify the placement and logic ofensure_cargo_purse_is_empty.Verification successful
The function
ensure_cargo_purse_is_emptyis designed to ensure that the cargo purse is empty and reverts if it is not. This function is used in two files:proxy_caller_with_return.rsandproxy_caller.rs. In both files, it is called after a proxy call is made.Observations:
- Definition: The function checks if the provided value is zero. If not, it retrieves the cargo purse balance and reverts if the balance is not zero.
- Usage in
proxy_caller_with_return.rs: The function is called after making a call to a versioned contract and before setting the result key.- Usage in
proxy_caller.rs: The function is called after making a call to a versioned contract.Conclusion:
The placement of
ensure_cargo_purse_is_emptyappears to be correct in both files. It is called after the proxy call, which is a logical point to ensure that the cargo purse is empty.Actionable Directive:
- The placement and logic of
ensure_cargo_purse_is_emptyare verified and correct.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `ensure_cargo_purse_is_empty` is called correctly and only when necessary. # Test: Search for the function usage. Expect: Only occurances where it's necessary. rg --type rust $'ensure_cargo_purse_is_empty'Length of output: 613
Script:
#!/bin/bash # Description: Gather the definition of `ensure_cargo_purse_is_empty` and its usage context in the relevant files. # Get the definition of `ensure_cargo_purse_is_empty` rg --type rust --context 10 'pub fn ensure_cargo_purse_is_empty' odra-casper/proxy-caller/src/lib.rs # Get the usage context in `proxy_caller_with_return.rs` rg --type rust --context 10 'ensure_cargo_purse_is_empty' odra-casper/proxy-caller/bin/proxy_caller_with_return.rs # Get the usage context in `proxy_caller.rs` rg --type rust --context 10 'ensure_cargo_purse_is_empty' odra-casper/proxy-caller/bin/proxy_caller.rsLength of output: 2460
odra-casper/proxy-caller/src/lib.rs (1)
119-128: Ensure correct implementation and usage ofensure_cargo_purse_is_empty.The implementation of
ensure_cargo_purse_is_emptycorrectly checks if the attached value is non-zero and then checks the balance of the cargo purse. This aligns with the PR's goal to prevent unintended token transfers.odra-vm/src/vm/odra_vm.rs (3)
79-82: Refactor error handling incall_contract.The refactoring of error handling in
call_contractsimplifies the flow and makes it more robust by directly handling errors and successful calls.
361-371: Simplify snapshot management inhandle_call_result.The changes to
handle_call_resultsimplify snapshot management, making the VM's state management more efficient and less error-prone.
488-490: Ensure proper error handling in test blocks.The addition of
catch_unwindin test blocks is a good practice to ensure that the VM handles errors gracefully and does not crash unexpectedly during contract calls.Also applies to: 508-510
…raVm
Summary by CodeRabbit
New Features
is_payableattribute to entry points, allowing differentiation between payable and non-payable functions.Bug Fixes
Tests
Refactor
Chores