digital cash sc payment migration #2244
Conversation
|
Contract comparison - from 5fb0bf2 to ab534f4
|
There was a problem hiding this comment.
Pull request overview
This PR migrates the digital-cash smart contract to use the new MultiversX payment APIs and transitions from block round-based to timestamp-based expiration tracking.
Changes:
- Migrated from
EgldOrEsdtTokenIdentifier/EgldOrEsdtTokenPaymenttypes toTokenId/Paymenttypes throughout the codebase - Changed expiration tracking from block rounds (
valability/expiration_round) to timestamps (expirationin milliseconds) - Renamed
blacklistFeeTokenendpoint toremoveFeeTokenfor better clarity - Reorganized error message constants into a dedicated
digital_cash_err_msg.rsmodule - Updated all test scenarios to reflect the new timestamp-based expiration model
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
wasm/src/lib.rs |
Updated endpoint mapping from blacklistFeeToken to removeFeeToken |
tests/digital_cash_scenario_rs_test.rs |
Reordered test functions alphabetically |
tests/digital_cash_scenario_go_test.rs |
Reordered test functions alphabetically |
src/storage.rs |
Migrated storage mappers to use TokenId instead of EgldOrEsdtTokenIdentifier |
src/signature_operations.rs |
Updated to use Payment type and timestamp-based expiration checks |
src/pay_fee_and_fund.rs |
Migrated payment handling to new API and timestamp-based expiration |
src/helpers.rs |
Added helper functions for fee calculation and migrated to new payment types |
src/digital_cash_proxy.rs |
Updated proxy signatures to use TokenId and TimestampMillis types |
src/digital_cash_err_msg.rs |
New file containing error message constants |
src/digital_cash.rs |
Updated init and endpoint signatures, migrated fee collection logic |
src/deposit_info.rs |
Updated structs to use Payment and TimestampMillis |
src/constants.rs |
Deleted file, constants moved to digital_cash_err_msg.rs |
scenarios/*.scen.json |
Updated all scenario files with timestamp-based expiration and new token identifiers |
README.md |
Partially updated documentation (incomplete migration noted in comments) |
Cargo.toml |
Minor formatting fix for authors field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) { | ||
| let paid_fee = self.call_value().egld_or_single_esdt(); | ||
| let additional_fee_payment = self.call_value().single_optional(); | ||
| let caller_address = self.blockchain().get_caller(); |
There was a problem hiding this comment.
The variable name caller_address is misleading in this context. The variable is only used to pass to update_fees but is actually cloned from self.blockchain().get_caller() before the closure. Inside the closure (lines 104-127), the variable is captured by value and used in line 107, but it represents the caller at the time of the forward call, not necessarily related to the deposit being updated. Consider renaming it to forward_caller or original_caller for clarity.
| require!( | ||
| self.blockchain().get_caller() == depositor, |
There was a problem hiding this comment.
The error message "invalid depositor" should be updated to "invalid depositor address" or more descriptively "only the original depositor can add funds to this deposit" to be more clear and helpful to users.
| fn whitelist_blacklist_fee_tokens_go() { | ||
| world().run("scenarios/whitelist-blacklist-fee-tokens.scen.json"); |
There was a problem hiding this comment.
Test function renamed from whitelist_blacklist_fee_token_go to whitelist_blacklist_fee_tokens_go. Note that the function name contains "blacklist" but the actual endpoint has been renamed to removeFeeToken. Consider renaming to whitelist_remove_fee_tokens_go for consistency with the new endpoint naming.
| fn whitelist_blacklist_fee_tokens_go() { | |
| world().run("scenarios/whitelist-blacklist-fee-tokens.scen.json"); | |
| fn whitelist_remove_fee_tokens_go() { | |
| world().run("scenarios/whitelist-remove-fee-tokens.scen.json"); |
| Each deposit is stored at a specific address (the check address). To claim funds: | ||
| 1. The recipient must provide a valid ED25519 signature proving they control the check address's private key | ||
| 2. The deposit must not have expired | ||
| 3. Fees must have been paid upfront by the depositor |
There was a problem hiding this comment.
The documentation states that deposits are stored at "a specific address (the check address)" and that recipients need to "prove ownership of the check address's private key". However, the actual implementation uses a DepositKey which is a 32-byte array (ManagedByteArray<M, 32>), not necessarily an address. The documentation should clarify that the deposit key is a 32-byte value used for ED25519 signature verification, which may or may not correspond to a blockchain address.
| - `address`: The check address containing the deposit | ||
| - `signature`: ED25519 signature proving ownership of the check's private key |
There was a problem hiding this comment.
The documentation states the parameter is named address, but the actual parameter name in the code is deposit_key. The documentation should be updated to use deposit_key to match the actual API.
| ## Withdrawing Funds | ||
|
|
||
| If a deposit has expired and hasn't been claimed, the original depositor can reclaim everything by calling `withdraw`: | ||
| - `address`: The check address containing the expired deposit |
There was a problem hiding this comment.
The documentation states the parameter is named address, but the actual parameter name in the code is deposit_key. The documentation should be updated to use deposit_key to match the actual API.
| - `address`: The current check address (source) | ||
| - `forward_address`: The new check address (destination) | ||
| - `signature`: ED25519 signature proving ownership of the source check |
There was a problem hiding this comment.
The documentation states the parameters are named address and forward_address, but the actual parameter names in the code are deposit_key and forward_deposit_key. The documentation should be updated to match the actual API.
| fn whitelist_blacklist_fee_tokens_rs() { | ||
| world().run("scenarios/whitelist-blacklist-fee-tokens.scen.json"); |
There was a problem hiding this comment.
Test function renamed from whitelist_blacklist_fee_token_rs to whitelist_blacklist_fee_tokens_rs. Note that the function name contains "blacklist" but the actual endpoint has been renamed to removeFeeToken. Consider renaming to whitelist_remove_fee_tokens_rs for consistency with the new endpoint naming.
| fn whitelist_blacklist_fee_tokens_rs() { | |
| world().run("scenarios/whitelist-blacklist-fee-tokens.scen.json"); | |
| fn whitelist_remove_fee_tokens_rs() { | |
| world().run("scenarios/whitelist-remove-fee-tokens.scen.json"); |
| - `address`: The check address where funds will be stored | ||
| - `expiration`: Timestamp in milliseconds when the deposit expires |
There was a problem hiding this comment.
The documentation states the parameter is named address, but the actual parameter name in the code is deposit_key. The documentation should be updated to use deposit_key to match the actual API.
| } | ||
|
|
||
| #[endpoint(depositFees)] | ||
| #[payable("EGLD")] |
There was a problem hiding this comment.
The #[payable("EGLD")] annotation restricts this endpoint to only accept EGLD payments. However, the contract supports multiple whitelisted fee tokens. This annotation should be removed or changed to #[payable] to allow any whitelisted token to be used for fee deposits, as the get_fee_for_token validation will ensure only whitelisted tokens are accepted.
| #[payable("EGLD")] | |
| #[payable] |
No description provided.