PoW Cw721 #697
PoW Cw721 #697
Conversation
WalkthroughThis pull request introduces a new non-fungible token (NFT) implementation within the Andromeda framework. It includes the creation of a new Cargo package with a configuration file that defines command aliases to streamline development tasks. The implementation features a smart contract with functions for instantiation, execution, querying, and migration, alongside a set of mock testing utilities. Additionally, it defines various message structures and response types for handling NFT operations, enhancing the overall functionality and organization of the codebase. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 8
🧹 Outside diff range and nitpick comments (21)
contracts/non-fungible-tokens/andromeda-pow-cw721/src/lib.rs (2)
6-7: Consider making the testing module private.The testing module is marked as
pub, but test utilities typically don't need to be publicly exposed unless they're intended to be used by other crates.#[cfg(test)] -pub mod testing; +mod testing;
1-12: Consider adding module documentation.While the module structure is clean, adding documentation comments for each module would improve maintainability and help other developers understand the PoW NFT implementation details.
Add documentation like this:
+//! Proof of Work NFT implementation that simulates Bitcoin-style mining. +//! Allows users to level up NFTs by solving increasingly difficult challenges. + pub mod contract;contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/mock_querier.rs (2)
18-20: Enhance function documentation.Consider adding more details about the return value and parameter requirements in the documentation.
/// Alternative to `cosmwasm_std::testing::mock_dependencies` that allows us to respond to custom queries. /// /// Automatically assigns a kernel address as MOCK_KERNEL_CONTRACT. +/// +/// # Arguments +/// * `contract_balance` - Initial balance for the mock contract +/// +/// # Returns +/// `OwnedDeps` instance with custom WasmMockQuerier implementation
33-48: Consider improving error handling in ADOContract instantiation.The
unwrap()call on the instantiation result could mask important error details during testing.- ADOContract::default() - .instantiate( - &mut deps.storage, - mock_env(), - &deps.api, - &QuerierWrapper::new(&deps.querier), - mock_info("sender", &[]), - InstantiateMsg { - ado_type: "pow-cw721".to_string(), - ado_version: "test".to_string(), - kernel_address: MOCK_KERNEL_CONTRACT.to_string(), - owner: None, - }, - ) - .unwrap(); + ADOContract::default() + .instantiate( + &mut deps.storage, + mock_env(), + &deps.api, + &QuerierWrapper::new(&deps.querier), + mock_info("sender", &[]), + InstantiateMsg { + ado_type: "pow-cw721".to_string(), + ado_version: "test".to_string(), + kernel_address: MOCK_KERNEL_CONTRACT.to_string(), + owner: None, + }, + ) + .map_err(|e| panic!("Failed to instantiate ADOContract: {}", e))?;contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/tests.rs (4)
16-26: Enhance instantiation test coverage.While the basic instantiation test is good, consider adding tests for:
- Instantiation with authorized minters
- Default state validation
- Error cases with invalid CW721 address
#[test] fn test_instantiation_with_minters() { let minters = vec![ AndrAddr::from_string(AUTHORIZED_ORIGIN_MINTER1), AndrAddr::from_string(AUTHORIZED_ORIGIN_MINTER2), ]; let deps = proper_initialization(AndrAddr::from_string(MOCK_CW721_CONTRACT), Some(minters.clone())); // Verify minters are stored correctly let stored_minters = query_authorized_minters(deps.as_ref()).unwrap(); assert_eq!(stored_minters, minters); }
28-51: Add edge cases to minting authorization tests.Consider adding tests for:
- Minting with empty authorized minters list
- Minting with invalid token_id format
- Minting the same token_id twice
#[test] fn test_mint_pow_nft_duplicate_token() { let mut deps = proper_initialization(AndrAddr::from_string(MOCK_CW721_CONTRACT), None); // First mint should succeed mint_pow_nft( deps.as_mut(), ORIGIN_MINTER, AndrAddr::from_string(ORIGIN_MINTER), "test_pow1".to_string(), None, TokenExtension { publisher: "Andromeda".to_string(), }, 10_u64, ) .unwrap(); // Second mint with same token_id should fail let err = mint_pow_nft( deps.as_mut(), ORIGIN_MINTER, AndrAddr::from_string(ORIGIN_MINTER), "test_pow1".to_string(), None, TokenExtension { publisher: "Andromeda".to_string(), }, 10_u64, ) .unwrap_err(); assert!(matches!(err, ContractError::TokenIdAlreadyExists { .. })); }
53-89: Enhance NFT state validation in minting test.The test should verify the complete NFT state after minting:
- Initial difficulty value
- Hash initialization
- TokenExtension data persistence
- Other metadata fields
Also, consider documenting why the initial difficulty is set to 10 or making it a named constant for better maintainability.
1-174: Consider enhancing test architecture and documentation.The test suite would benefit from:
- A test helper module to calculate valid nonces and expected difficulties
- Documentation explaining the PoW algorithm and difficulty calculation
- Property-based testing for proof validation
- Integration tests for concurrent operations
Consider using a test helper like this:
mod test_helpers { /// Calculates a valid nonce for the given difficulty pub fn calculate_valid_nonce(difficulty: u64, previous_hash: &str) -> u128 { // Add implementation } /// Calculates expected difficulty for a given level pub fn calculate_difficulty(initial_difficulty: u64, level: u32) -> u64 { // Add implementation using 150% increase formula } }packages/andromeda-non-fungible-tokens/src/pow_cw721.rs (3)
32-32: Consider changingownertype toAndrAddrfor consistency.To maintain consistency with the
MintPowNFTstruct, consider changing theownerfield inPowNFTInfofromAddrtoAndrAddr.
50-50: Renamenft_responsetopow_nft_infofor clarity.In the
GetPowNFTResponsestruct, renaming the fieldnft_responsetopow_nft_infocan improve readability by clearly indicating that it containsPowNFTInfo.Apply this diff to make the change:
pub struct GetPowNFTResponse { - pub nft_response: PowNFTInfo, + pub pow_nft_info: PowNFTInfo, }
55-55: Consider renaminglinked_cw721_addresstolinked_cw721_addrfor consistency.Across the codebase, shorter variable names like
addrare commonly used. Renaminglinked_cw721_addresstolinked_cw721_addrcan enhance consistency.contracts/non-fungible-tokens/andromeda-pow-cw721/src/contract.rs (1)
95-97: Directly returning error without handling may cause issues.In the
replyfunction, you are directly returning the error frommsg.result.unwrap_err(). Consider mapping the error to a more descriptiveContractErroror handling specific error cases for better clarity.Apply this diff to improve error handling:
if msg.result.is_err() { - return Err(ContractError::Std(StdError::generic_err( - msg.result.unwrap_err(), - ))); + let error_msg = format!("Reply failed: {:?}", msg.result.unwrap_err()); + return Err(ContractError::Std(StdError::generic_err(error_msg))); }contracts/non-fungible-tokens/andromeda-pow-cw721/src/state.rs (1)
5-6: Ensure proper namespacing of storage keys.To prevent key collisions in storage, consider prefixing the storage keys with a unique namespace or module identifier.
Apply this diff to update the storage keys:
pub const LINKED_CW721_ADDRESS: Item<AndrAddr> = Item::new("linked_cw721_address"); pub const POW_NFT: Map<String, PowNFTInfo> = Map::new("pow_nft"); +// For example: +pub const LINKED_CW721_ADDRESS: Item<AndrAddr> = Item::new("pow_cw721_linked_cw721_address"); +pub const POW_NFT: Map<String, PowNFTInfo> = Map::new("pow_cw721_pow_nft");This change reduces the risk of key collisions with other modules or contracts.
contracts/non-fungible-tokens/andromeda-pow-cw721/src/query.rs (2)
7-15: Add documentation for the query_pow_nft function.Consider adding documentation that explains:
- The purpose of the function
- The expected format of token_id
- The error conditions
Example documentation:
/// Queries information about a specific PoW NFT /// /// # Arguments /// * `deps` - Dependencies for storage access /// * `token_id` - Unique identifier of the NFT /// /// # Returns /// * `GetPowNFTResponse` containing the NFT information /// * `ContractError::NFTNotFound` if the token doesn't exist
17-25: Add documentation for the query_linked_cw721_address function.Consider adding documentation that explains:
- The purpose of the function
- The relationship with the linked CW721 contract
Example documentation:
/// Retrieves the address of the linked CW721 contract /// /// # Arguments /// * `deps` - Dependencies for storage access /// /// # Returns /// * `GetLinkedCw721AddressResponse` containing the linked contract address /// * Propagates any storage errorscontracts/non-fungible-tokens/andromeda-pow-cw721/Cargo.toml (2)
34-34: Consider using workspace version for sha2 dependency.Other dependencies use workspace versions for consistency. Consider moving sha2 to workspace dependencies as well.
36-36: Clean up commented out dependencies.Remove commented out dependencies if they're not needed:
hex = { version = "0.4.3" }cw-orch-daemon = "0.24.2"Also applies to: 45-45
contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/mock.rs (4)
1-1: Remove or document the commented import.The commented import
andromeda_data_storage::graphsuggests either planned functionality or leftover code. If it's not needed, please remove it. If it's for future use, add a TODO comment explaining the intended usage.
20-35: Consider adding validation and documentation.While the initialization looks correct, consider:
- Adding validation for empty
authorized_origin_minter_addressesvector- Documenting why
owner: Noneis acceptable in the test contextpub fn proper_initialization( linked_cw721_address: AndrAddr, authorized_origin_minter_addresses: Option<Vec<AndrAddr>>, ) -> MockDeps { + // Validate that if authorized_origin_minter_addresses is Some, it's not empty + if let Some(addresses) = &authorized_origin_minter_addresses { + assert!(!addresses.is_empty(), "Minter addresses vector cannot be empty"); + } let mut deps = mock_dependencies_custom(&[]); let info = mock_info("creator", &[]); let msg = InstantiateMsg { kernel_address: MOCK_KERNEL_CONTRACT.to_string(), + // Owner is None in test context as it's handled by kernel contract owner: None, linked_cw721_address, authorized_origin_minter_addresses, };
57-66: Document the proof verification process.Since this implements a PoW mechanism where difficulty increases by 150% per level, it would be helpful to document:
- How the proof is verified
- The difficulty increase mechanism
- The expected format/calculation of the nonce
+/// Submits a proof of work for a token to increase its level +/// The proof is verified by: +/// 1. Calculating hash(previous_hash + nonce) +/// 2. Checking if the hash meets the current difficulty requirement +/// 3. Difficulty increases by 150% for each level +/// The nonce should be calculated to produce a hash with the required number of leading zeros pub fn submit_proof( deps: DepsMut<'_>, sender: &str, token_id: String, nonce: u128, ) -> Result<Response, ContractError> {
68-84: Simplify error handling and add return type documentation.The query functions could be improved for better maintainability and clarity.
+/// Returns the linked CW721 contract address +/// Returns GetLinkedCw721AddressResponse containing the address pub fn query_linked_cw721_address( deps: Deps, ) -> Result<GetLinkedCw721AddressResponse, ContractError> { - let res = query(deps, mock_env(), QueryMsg::GetLinkedCw721Address {}); - match res { - Ok(res) => Ok(from_json(res).unwrap()), - Err(err) => Err(err), - } + query(deps, mock_env(), QueryMsg::GetLinkedCw721Address {}) + .map_err(|e| e) + .and_then(|res| Ok(from_json(res)?)) } +/// Returns information about a specific PoW NFT +/// Returns GetPowNFTResponse containing the NFT's level, difficulty, and last hash pub fn query_pow_nft(deps: Deps, token_id: String) -> Result<GetPowNFTResponse, ContractError> { - let res = query(deps, mock_env(), QueryMsg::GetPowNFT { token_id }); - match res { - Ok(res) => Ok(from_json(res).unwrap()), - Err(err) => Err(err), - } + query(deps, mock_env(), QueryMsg::GetPowNFT { token_id }) + .map_err(|e| e) + .and_then(|res| Ok(from_json(res)?)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
contracts/non-fungible-tokens/andromeda-pow-cw721/.cargo/config(1 hunks)contracts/non-fungible-tokens/andromeda-pow-cw721/Cargo.toml(1 hunks)contracts/non-fungible-tokens/andromeda-pow-cw721/examples/schema.rs(1 hunks)contracts/non-fungible-tokens/andromeda-pow-cw721/src/contract.rs(1 hunks)contracts/non-fungible-tokens/andromeda-pow-cw721/src/execute.rs(1 hunks)contracts/non-fungible-tokens/andromeda-pow-cw721/src/interface.rs(1 hunks)contracts/non-fungible-tokens/andromeda-pow-cw721/src/lib.rs(1 hunks)contracts/non-fungible-tokens/andromeda-pow-cw721/src/query.rs(1 hunks)contracts/non-fungible-tokens/andromeda-pow-cw721/src/state.rs(1 hunks)contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/mock.rs(1 hunks)contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/mock_querier.rs(1 hunks)contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/mod.rs(1 hunks)contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/tests.rs(1 hunks)packages/andromeda-non-fungible-tokens/src/lib.rs(1 hunks)packages/andromeda-non-fungible-tokens/src/pow_cw721.rs(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/andromeda-non-fungible-tokens/src/lib.rs
- contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/mod.rs
- contracts/non-fungible-tokens/andromeda-pow-cw721/examples/schema.rs
🔇 Additional comments (9)
contracts/non-fungible-tokens/andromeda-pow-cw721/src/lib.rs (2)
1-4: LGTM! Well-organized module structure.
The modular organization follows best practices for Cosmos SDK contracts, with clear separation of concerns:
contract: Entry points and core logicexecute: Message execution handlersquery: Query handlersstate: Contract state management
9-12: Verify WASM target exclusion for interface module.
The interface module and PowCw721Contract are correctly excluded from WASM builds. However, let's verify that this contract is properly integrated with other non-WASM components that might depend on it.
✅ Verification successful
Based on the gathered information, I can now provide a final response:
WASM target exclusion is correctly implemented and safe.
The interface module and PowCw721Contract are properly excluded from WASM builds through the #[cfg(not(target_arch = "wasm32"))] attribute. The code shows:
- The interface is only used for testing and local development:
- It's part of the non-WASM test infrastructure along with
cw-multi-testandandromeda-testing - All test files and mock implementations are correctly excluded from WASM builds
- It's part of the non-WASM test infrastructure along with
- The Cargo.toml configuration properly handles the separation:
- Test dependencies are correctly gated behind
[target.'cfg(not(target_arch = "wasm32"))'.dependencies] - WASM-specific artifacts are properly excluded in the package configuration
- Test dependencies are correctly gated behind
No issues were found with the WASM target exclusion implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any direct usage of PowCw721Contract in WASM-targeted code
# which could cause compilation errors
# Search for PowCw721Contract usage
rg "PowCw721Contract" --type rust
# Look for any potential WASM integration tests or examples
fd -e rs -e toml . | xargs rg "target_arch.*wasm32.*PowCw721Contract"
Length of output: 357
Script:
#!/bin/bash
# Let's check the interface module implementation and its usage patterns
# First, let's see the interface.rs content
cat contracts/non-fungible-tokens/andromeda-pow-cw721/src/interface.rs
# Check for any test modules or integration tests
fd -e rs . -t f | grep -i "test" | xargs cat
# Look for any dependencies in Cargo.toml that might use this contract
cat contracts/non-fungible-tokens/andromeda-pow-cw721/Cargo.toml
# Check for any potential cross-contract calls or imports
rg "use.*andromeda.*pow.*cw721" --type rust
Length of output: 109748
Script:
#!/bin/bash
# Let's check for any WASM-specific code or dependencies in the codebase
# First, check for any WASM-specific dependencies in Cargo.toml
cat contracts/non-fungible-tokens/andromeda-pow-cw721/Cargo.toml
# Check for any WASM-specific imports or uses in the codebase
rg "wasm|target_arch" --type rust contracts/non-fungible-tokens/andromeda-pow-cw721/
# Check for any test files that might verify WASM integration
fd -e rs . contracts/non-fungible-tokens/andromeda-pow-cw721/ | grep -i test | xargs cat
Length of output: 15862
contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/mock_querier.rs (2)
1-17: LGTM! Well-organized imports and constants.
The imports are logically grouped and the constants are appropriately named for mock testing purposes.
50-99: Add test coverage for error cases.
The mock querier implementation would benefit from additional test coverage to verify error handling paths.
Let's check the current test coverage:
Would you like me to help generate comprehensive test cases for the mock querier error paths?
contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/tests.rs (2)
1-14: LGTM! Well-organized test setup.
The imports and constants provide a clear foundation for testing different minter scenarios.
91-144: 🛠️ Refactor suggestion
Document proof validation logic and enhance test coverage.
The test cases use hardcoded nonce values without explaining how they were calculated or why they are valid/invalid. This makes it difficult to understand the proof validation logic.
- Add documentation explaining the proof validation algorithm and how valid nonces are calculated.
- Add tests for:
- Concurrent proof submissions
- Difficulty increase after successful proof
- Edge cases near the difficulty threshold
/// Demonstrates how valid nonces are calculated
#[test]
fn test_proof_calculation_explanation() {
// Add a test that shows step by step how a valid nonce is calculated
// and why it meets the difficulty requirement
}
/// Tests concurrent proof submissions
#[test]
fn test_concurrent_proof_submissions() {
// Add a test that simulates multiple users submitting proofs
// for the same NFT concurrently
}Let's verify the proof validation implementation:
contracts/non-fungible-tokens/andromeda-pow-cw721/.cargo/config (1)
1-4: LGTM!
The Cargo aliases are correctly defined and will help streamline development tasks.
contracts/non-fungible-tokens/andromeda-pow-cw721/src/interface.rs (1)
1-6: LGTM! Clean and well-structured interface implementation.
The interface follows standard Cosmos-SDK patterns and includes all necessary imports from the Andromeda ecosystem.
contracts/non-fungible-tokens/andromeda-pow-cw721/Cargo.toml (1)
1-48: Verify security of dependencies.
Let's check for any known vulnerabilities in the dependencies.
✅ Verification successful
Dependencies are using secure versions - no known vulnerabilities found
The key dependencies are using recent stable versions:
- cosmwasm-std v1.5.8 (latest stable)
- sha2 v0.10.8 (latest stable)
The codebase shows appropriate security awareness through documentation and comments. No security advisories or known vulnerabilities were found for the specified versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security advisories in dependencies
# Check for security advisories in cosmwasm-std
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: CARGO, package: "cosmwasm-std") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
# Check for security advisories in sha2
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: CARGO, package: "sha2") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 1543
Script:
#!/bin/bash
# Let's try a different approach to check dependencies
# First, let's check if there's a Cargo.lock file to see exact versions
fd -t f "Cargo.lock" --exec cat {}
# Let's also check workspace dependencies in the root Cargo.toml
cat Cargo.toml
# Let's also look for any security-related comments or known issues in the codebase
rg -i "security|vulnerability|advisory" -g "*.{md,toml}"
Length of output: 65678
Script:
#!/bin/bash
# Let's check for specific versions of key dependencies and any security-related comments
# Check workspace dependencies in root Cargo.toml
rg -A 5 '\[workspace\.dependencies\]' Cargo.toml
# Check for any security advisories in README or other docs
rg -i "security|vulnerability|advisory" -g "*.md"
# Check specific versions of security-critical dependencies
rg -A 1 'sha2.*=|cosmwasm-std.*=' Cargo.toml
# Look for any security-related comments in the code
rg -i "security|vulnerability|advisory" -g "*.rs"
Length of output: 2973
contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/mock_querier.rs
Show resolved
Hide resolved
contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/tests.rs
Outdated
Show resolved
Hide resolved
| #[cw_serde] | ||
| pub enum ExecuteMsg { | ||
| MintPowNFT { | ||
| owner: AndrAddr, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent address types for owner fields.
The owner field in MintPowNFT is of type AndrAddr, whereas in PowNFTInfo, the owner field is of type Addr. For consistency and to prevent potential type conversion errors, consider using the same address type for both fields. If AndrAddr provides additional functionality required elsewhere, you may convert it to Addr when necessary.
| pow_nft.difficulty = if pow_nft.difficulty >= 2 { | ||
| let next_difficulty = (pow_nft.difficulty as f64 * 1.5) as u64; | ||
| if next_difficulty > 128 { | ||
| return Err(ContractError::CustomError { | ||
| msg: format!( | ||
| "Max difficulty is 128. Next difficulty will be over 128. Current level: {:?}", | ||
| pow_nft.level | ||
| ), | ||
| }); | ||
| } | ||
| next_difficulty | ||
| } else { |
There was a problem hiding this comment.
Avoid using floating-point arithmetic in smart contracts.
The difficulty adjustment uses floating-point arithmetic, which can lead to non-deterministic behavior and precision issues in smart contracts. It's recommended to use integer arithmetic to ensure deterministic and precise calculations.
Apply this diff to adjust the difficulty calculation:
pow_nft.difficulty = if pow_nft.difficulty >= 2 {
- let next_difficulty = (pow_nft.difficulty as f64 * 1.5) as u64;
+ let next_difficulty = pow_nft.difficulty * 3 / 2;
if next_difficulty > 128 {
return Err(ContractError::CustomError {
msg: format!(This change replaces the floating-point multiplication with integer arithmetic, ensuring consistent behavior across different platforms.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pow_nft.difficulty = if pow_nft.difficulty >= 2 { | |
| let next_difficulty = (pow_nft.difficulty as f64 * 1.5) as u64; | |
| if next_difficulty > 128 { | |
| return Err(ContractError::CustomError { | |
| msg: format!( | |
| "Max difficulty is 128. Next difficulty will be over 128. Current level: {:?}", | |
| pow_nft.level | |
| ), | |
| }); | |
| } | |
| next_difficulty | |
| } else { | |
| pow_nft.difficulty = if pow_nft.difficulty >= 2 { | |
| let next_difficulty = pow_nft.difficulty * 3 / 2; | |
| if next_difficulty > 128 { | |
| return Err(ContractError::CustomError { | |
| msg: format!( | |
| "Max difficulty is 128. Next difficulty will be over 128. Current level: {:?}", | |
| pow_nft.level | |
| ), | |
| }); | |
| } | |
| next_difficulty | |
| } else { |
| let hash_value = u128::from_be_bytes(hash[0..16].try_into().unwrap()); | ||
| let threshold = u128::MAX >> (pow_nft.difficulty as u32); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential panics when converting hash bytes to u128.
The use of unwrap() in hash[0..16].try_into().unwrap() can cause a panic if the slice does not have the correct length. Consider handling the potential error using ? to propagate it properly.
Apply this diff to handle the error:
let hash_value = u128::from_be_bytes(hash[0..16].try_into().unwrap());
+// Change to:
+let hash_value = u128::from_be_bytes(hash[0..16].try_into().map_err(|_| ContractError::CustomError {
+ msg: "Failed to convert hash bytes to u128.".to_string(),
+})?);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
30-30: Consider expanding the changelog entry with implementation details.While the entry follows the correct format, consider adding more details about the key features, such as:
- NFT leveling mechanics
- 150% difficulty increase per level
- Proof of Work challenge system
This would help users and developers better understand the significant features introduced.
Example expansion:
-Added PoW Cw721 ADO [(#697)](https://github.com/andromedaprotocol/andromeda-core/pull/697) +Added PoW Cw721 ADO with NFT leveling system, 150% difficulty scaling, and Bitcoin-style PoW challenges [(#697)](https://github.com/andromedaprotocol/andromeda-core/pull/697)
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)contracts/non-fungible-tokens/andromeda-pow-cw721/src/execute.rs(1 hunks)contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/mock.rs(1 hunks)packages/andromeda-non-fungible-tokens/src/pow_cw721.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/andromeda-non-fungible-tokens/src/pow_cw721.rs
- CHANGELOG.md
- contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/mock.rs
🔇 Additional comments (3)
contracts/non-fungible-tokens/andromeda-pow-cw721/src/execute.rs (3)
23-23:
Remove incorrect usage of as_ref()
The ExecuteMsg enum doesn't implement AsRef, making this call invalid.
-msg.as_ref(),
+&msg,125-136:
Replace floating-point arithmetic with integer operations
Using floating-point arithmetic in smart contracts can lead to non-deterministic behavior.
116-118:
Handle potential panics in hash conversion
The unwrap usage can cause panics if the slice conversion fails.
contracts/non-fungible-tokens/andromeda-pow-cw721/src/execute.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
contracts/non-fungible-tokens/andromeda-pow-cw721/src/execute.rs(1 hunks)contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/tests.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/tests.rs
🔇 Additional comments (6)
contracts/non-fungible-tokens/andromeda-pow-cw721/src/execute.rs (6)
1-16: LGTM! All necessary imports are present and well-organized.
The imports cover all required functionality including Cosmos SDK, Andromeda framework, and cryptographic operations.
54-58: LGTM! Proper validation of base_difficulty parameter.
The implementation correctly validates that base_difficulty is within acceptable bounds (1-128).
23-24:
Remove incorrect as_ref() usage
The ExecuteMsg enum doesn't implement AsRef, making this call invalid. This needs to be fixed to ensure proper message handling.
- msg.as_ref(),
+ &msg,86-91: 🛠️ Refactor suggestion
Enhance initial hash entropy
Using only block height for the initial hash makes it predictable. Consider adding more entropy sources.
let block_height = ctx.env.block.height;
+let block_time = ctx.env.block.time.nanos();
+let sender = ctx.info.sender.as_bytes();
let mut hasher = Sha256::new();
hasher.update(block_height.to_be_bytes());
+hasher.update(block_time.to_be_bytes());
+hasher.update(sender);
let last_hash = hasher.finalize().to_vec();131-142:
Replace floating-point arithmetic with integer operations
Using floating-point arithmetic in smart contracts can lead to non-deterministic behavior.
- let next_difficulty = (pow_nft.difficulty as f64 * 1.5) as u64;
+ let next_difficulty = pow_nft.difficulty * 3 / 2;122-124:
Replace unwrap() with proper error handling
Using unwrap() can cause panics. Implement proper error handling for the byte conversion.
- let hash_value = u128::from_be_bytes(hash[0..16].try_into().unwrap());
+ let hash_value = u128::from_be_bytes(hash[0..16].try_into().map_err(|_| ContractError::CustomError {
+ msg: "Failed to convert hash bytes to u128".to_string(),
+ })?);Likely invalid or redundant comment.
contracts/non-fungible-tokens/andromeda-pow-cw721/src/execute.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
contracts/non-fungible-tokens/andromeda-pow-cw721/src/execute.rs (1)
131-144: Consider implementing difficulty retargeting mechanismThe current implementation increases difficulty by a fixed percentage. Consider implementing a more sophisticated difficulty adjustment algorithm that takes into account:
- Time between proof submissions
- Network hash rate
- Target block time (like Bitcoin's 10-minute target)
This would help maintain consistent proof-of-work intervals regardless of network computing power.
Would you like me to provide an implementation of a more sophisticated difficulty adjustment algorithm?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
contracts/non-fungible-tokens/andromeda-pow-cw721/src/execute.rs(1 hunks)
🔇 Additional comments (1)
contracts/non-fungible-tokens/andromeda-pow-cw721/src/execute.rs (1)
23-24:
Replace msg.as_ref() with proper message serialization
The ExecuteMsg type doesn't implement AsRef, so this will fail to compile.
-msg.as_ref(),
+&msg,
contracts/non-fungible-tokens/andromeda-pow-cw721/src/execute.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
contracts/non-fungible-tokens/andromeda-pow-cw721/src/execute.rs (3)
37-37: Improve error handling for unknown message variantsInstead of delegating unknown variants to
ADOContract::default().execute(), consider explicitly handling all possible message variants to make the code more maintainable and prevent potential security issues.- _ => ADOContract::default().execute(ctx, msg), + ExecuteMsg::Other(msg) => ADOContract::default().execute(ctx, ExecuteMsg::Other(msg)),
110-113: Add event emission for NFT mintingThe response should include an event for the minting operation to help indexers track NFT creation.
Ok(Response::new() .add_message(mint_msg) .add_attribute("method", "mint_pow_nft") - .add_attribute("sender", sender)) + .add_attribute("sender", sender) + .add_attribute("token_id", token_id.clone()) + .add_attribute("base_difficulty", base_difficulty.to_string()) + .add_event( + Event::new("pow_nft_minted") + .add_attribute("token_id", token_id) + .add_attribute("owner", owner_addr.to_string()) + .add_attribute("base_difficulty", base_difficulty.to_string()) + ))
119-119: Consider using a larger integer type for the solution parameterUsing
u128for the solution might be limiting as the difficulty increases. Consider using a string or byte array to allow for arbitrary-length solutions in future updates.- solution: u128, + solution: String, // or Vec<u8>Note: This change would require adjusting the solution processing logic accordingly.
…-core into mdjakovic/pow-cw721
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
contracts/non-fungible-tokens/andromeda-pow-cw721/Cargo.toml (3)
34-34: Consider using workspace version for sha2Other dependencies use workspace versioning, but sha2 has a direct version specification. Consider moving this to the workspace level for consistent dependency management.
36-36: Remove commented out dependenciesThere are commented out dependencies that should be either removed or uncommented if needed:
- Line 36:
hex = { version = "0.4.3" }- Line 45:
cw-orch-daemon = "0.24.2"Also applies to: 45-45
27-40: Verify dependencies for PoW implementationThe current dependencies cover NFT functionality through cw721 and hashing through sha2. For a PoW implementation, consider if additional dependencies are needed for:
- Difficulty adjustment calculations
- Nonce validation
- Memory-hard hashing (optional, for ASIC resistance)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)contracts/non-fungible-tokens/andromeda-pow-cw721/Cargo.toml(1 hunks)
🔇 Additional comments (2)
contracts/non-fungible-tokens/andromeda-pow-cw721/Cargo.toml (2)
5-6: Verify Rust version requirement
The specified Rust version 1.75.0 is quite recent. Consider if this strict requirement is necessary or if the contract could work with an earlier stable version to ensure broader compatibility.
✅ Verification successful
Based on the output, I can see that all contracts in the workspace consistently use Rust version 1.75.0, with only one exception (andromeda-kernel using 1.65.0). Since this is a consistent pattern across the codebase and represents an intentional choice for the project, I can generate the final response:
Rust version requirement is consistent with workspace
The specified Rust version 1.75.0 aligns with the version used across most packages in the workspace. This appears to be an intentional standardization across the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other contracts in the workspace use different Rust versions
fd -e toml -x grep -l "rust-version" {} \; | xargs cat
Length of output: 60148
41-44: Verify test coverage with optional testing dependencies
Since cw-multi-test and andromeda-testing are marked as optional dependencies, ensure that the test suite provides adequate coverage when these features are disabled.
✅ Verification successful
Tests are properly implemented without optional dependencies
The contract has a comprehensive test suite that uses Cosmos-SDK's native testing utilities (cosmwasm_std::testing) instead of the optional dependencies. The tests include:
- Proper initialization tests
- Minting functionality tests
- Token-specific operations tests
- Custom mock querier implementation
The test implementation is robust without relying on cw-multi-test or andromeda-testing, making the optional nature of these dependencies appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for test files and their coverage
fd -e rs -x grep -l "#[test]" {} \;
Length of output: 91
Script:
#!/bin/bash
# Check test files in the specific contract directory
fd -e rs . contracts/non-fungible-tokens/andromeda-pow-cw721/ -x grep -l "#[test]"
# Check if there are any test modules or test functions
rg -l "mod tests" contracts/non-fungible-tokens/andromeda-pow-cw721/
rg "#\[test\]" contracts/non-fungible-tokens/andromeda-pow-cw721/ -A 1
# Check if there are any cfg test attributes
rg "cfg\(test\)" contracts/non-fungible-tokens/andromeda-pow-cw721/
Length of output: 1166
Script:
#!/bin/bash
# Check if tests use these optional dependencies
rg "use (cw_multi_test|andromeda_testing)" contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/
# Check for feature flags in test modules
rg "cfg\(feature" contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/
# Look at the test module setup
head -n 20 contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/mod.rs
Length of output: 311
Script:
#!/bin/bash
# Check the content of mock files to see testing setup
cat contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/mock.rs
cat contracts/non-fungible-tokens/andromeda-pow-cw721/src/testing/mock_querier.rs
# Check if cw-orch is used in tests
rg "use cw_orch" contracts/non-fungible-tokens/andromeda-pow-cw721/
Length of output: 6433
Motivation
The motivation of this ADO is to be able to simulate BTC style PoW mining with NFTs.
It takes very little work to solve the first one, and once it’s solved, the NFT increases its “level”.
Last block hash, nonce = seed for challenge.
Just have it where someone can submit the proof to the ADO and it levels up.
Level 1 -> 2 -> 3 -> 4 (etc)
But each level gets harder to solve.
Just make the difficulty go up by 150% or something, so that it eventually gets really difficult.
Then we can create a little JS that solves it and we can submit it via command line.
Implementation
Testing
Unit test cases are added.
Version Changes
Version is set as 0.1.0-beta
Future work
We can set the other methods to solve the challenge to level up.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation