feat: deprecating nft_exists mapping #130
Conversation
Benchmark ComparisonContract: token
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Benchmark ComparisonContract: token
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
waiting for nft benchmark |
Benchmark ComparisonContract: token
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
wei3erHase
changed the title
Benchmark ComparisonContract: token
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
… feat/deprecate-nft-exists
Benchmark ComparisonContract: nft
Contract: token
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
1 similar comment
Benchmark ComparisonContract: nft
Contract: token
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
📝 WalkthroughWalkthroughThis change removes the Estimated code review effort4 (~90 minutes) ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/nft_contract/src/main.nr(12 hunks)src/nft_contract/src/test/mint_to_private.nr(2 hunks)src/nft_contract/src/test/transfer_private_to_public.nr(3 hunks)src/nft_contract/src/test/transfer_public_to_private.nr(3 hunks)src/nft_contract/src/test/utils.nr(2 hunks)src/nft_contract/src/types.nr(1 hunks)src/nft_contract/src/types/constants.nr(1 hunks)src/ts/test/nft.test.ts(3 hunks)
🔇 Additional comments (20)
src/nft_contract/src/types/constants.nr (1)
1-4: LGTM! Well-designed sentinel constant.The
PRIVATE_OWNER_ADDRESSconstant is properly implemented using deterministic hashing of a descriptive string. This approach ensures a collision-resistant sentinel value that's distinct from legitimate addresses while being consistently reproducible across all usage contexts.src/nft_contract/src/types.nr (1)
2-2: LGTM! Proper module declaration.The addition of the
constantsmodule declaration correctly exposes the new constant definitions for use throughout the contract codebase.src/nft_contract/src/test/mint_to_private.nr (2)
3-3: LGTM! Proper import of the new constant.The import correctly brings in the
PRIVATE_OWNER_ADDRESSconstant needed for the updated test logic.
16-16: LGTM! Test updated to reflect new private ownership semantics.The assertion correctly verifies that after minting to private, the public owner is set to
PRIVATE_OWNER_ADDRESSinstead of the zero address, aligning with the new sentinel-based approach for indicating private ownership.src/nft_contract/src/test/transfer_public_to_private.nr (2)
3-3: LGTM! Proper import of the sentinel constant.The import correctly brings in
PRIVATE_OWNER_ADDRESSto replace the previous zero address usage in ownership assertions.
24-24: LGTM! Assertions updated for new private ownership semantics.Both test functions correctly verify that after transferring NFTs to private state, the public owner is set to
PRIVATE_OWNER_ADDRESSinstead of the zero address, properly reflecting the new sentinel-based approach.Also applies to: 53-53
src/nft_contract/src/test/transfer_private_to_public.nr (2)
3-3: LGTM! Proper import of the sentinel constant.The import correctly brings in
PRIVATE_OWNER_ADDRESSfor use in the updated test assertions.
15-15: LGTM! Initial state verifications updated correctly.Both assertions properly verify that NFTs in private state have
PRIVATE_OWNER_ADDRESSas their public owner before transferring to public state, correctly reflecting the new sentinel-based private ownership semantics.Also applies to: 40-40
src/ts/test/nft.test.ts (2)
19-22: LGTM! Constant definition aligns with contract implementation.The
PRIVATE_OWNER_ADDRESSconstant is correctly defined using the Poseidon hash of "PRIVATE_OWNER", matching the contract's implementation.
560-560: Correct usage of PRIVATE_OWNER_ADDRESS sentinel value.The assertions correctly expect
PRIVATE_OWNER_ADDRESSas the public owner when NFTs are transferred to private ownership, aligning with the new ownership model.Also applies to: 608-608
src/nft_contract/src/test/utils.nr (1)
87-101: Correct implementation of token existence check.The function properly determines token existence by checking if the public owner is non-zero, which aligns with the new ownership model after removing the
nft_existsmapping.src/nft_contract/src/main.nr (9)
36-37: Correct import of PRIVATE_OWNER_ADDRESS constant.The constant is properly imported to support the new ownership model.
93-94: Correct usage of PRIVATE_OWNER_ADDRESS in private-to-public transfers.The functions correctly update the public owner from
PRIVATE_OWNER_ADDRESSto the new owner, properly reflecting the transition from private to public ownership.Also applies to: 115-116
180-180: Correct implementation of public-to-private transfer.The function properly sets the public owner to
PRIVATE_OWNER_ADDRESSwhen transferring to private ownership, maintaining consistency with the new ownership model.Also applies to: 195-196
309-317: Well-implemented mint_private_internal function.The function correctly ensures the token doesn't exist (zero owner) before setting the owner to
PRIVATE_OWNER_ADDRESS, maintaining the ownership invariants.
319-326: Proper implementation of burn_from_private_internal.The function correctly validates that the token is privately owned (PRIVATE_OWNER_ADDRESS) before burning it by setting the owner to zero.
396-397: Correct implementation of private minting.The function properly uses
mint_private_internalto set the public owner toPRIVATE_OWNER_ADDRESS, indicating private ownership.
414-416: Proper token existence validation.The function correctly checks that the token doesn't exist (zero owner) before minting, preventing duplicate token IDs.
434-435: Correct implementation of private burning.The function properly uses
burn_from_private_internalto set the public owner to zero, indicating the token no longer exists.
36-37: Confirm PRIVATE_OWNER_ADDRESS is unreachable by any valid key derivationI see that
PRIVATE_OWNER_ADDRESSis computed as:pub global PRIVATE_OWNER_ADDRESS: AztecAddress = AztecAddress::from_field(poseidon2_hash_bytes("PRIVATE_OWNER".as_bytes()));However, security depends on this field value never being producible by a legitimate account’s address derivation. Please verify the following:
• AztecAddress derivation model
– Ensure normal user addresses are computed via a one-way hash of a public key (e.g., Blake2 or similar), not directly from arbitrary field inputs.
– Confirm thatAztecAddress::from_fieldis not used when deriving real user addresses.• Collision impossibility
– Validate that no public key → address function can ever output the Poseidon2 hash of"PRIVATE_OWNER".If both hold, the sentinel is cryptographically unreachable and safe to use. Otherwise, consider switching to an address derived from a one-time random private key (so its public address cannot be reproduced).
| _validate_public_token_owner(storage.public_owners, from, token_id); | ||
|
|
||
| _update_public_token_owner(storage.public_owners, AztecAddress::zero(), token_id); | ||
| _update_public_token_owner(storage.public_owners, PRIVATE_OWNER_ADDRESS, token_id); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential state confusion with commitment transfers.
Setting the public owner to PRIVATE_OWNER_ADDRESS during commitment transfers (line 266) could create ambiguity. This address is also used to indicate regular private ownership, making it impossible to distinguish between tokens in commitment state vs. regular private ownership from the public state alone.
Consider using a separate sentinel address for commitment states to maintain clear state separation.
🤖 Prompt for AI Agents
In src/nft_contract/src/main.nr at line 266, the code sets the public owner to
PRIVATE_OWNER_ADDRESS during commitment transfers, which causes ambiguity
between commitment state and regular private ownership. To fix this, define and
use a distinct sentinel address specifically for commitment states instead of
reusing PRIVATE_OWNER_ADDRESS, ensuring clear differentiation in the public
ownership state.
Benchmark ComparisonContract: nft
Contract: token
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
🤖 Linear
Closes AZT-225
Description
Summary by CodeRabbit
Refactor
Tests
New Features