-
Notifications
You must be signed in to change notification settings - Fork 104
Add reusable ownable module and refactor network fungible faucet to use it #2228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Conversation
onurinanc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @afa7789 for this work! I have few comments from my side. I believe @mmagician would provide a more detailed feedback related to ownable logic.
Also, for the folder structure such as how we would like to have the utils under the standards.
Additionally, from the discussion here: OpenZeppelin/miden-confidential-contracts#34 (comment)
Is it possible to design an address, which is not usable for anyone such as address(0)?
| # | ||
| # See the `NetworkFungibleFaucet` Rust type's documentation for more details. | ||
|
|
||
| use miden::standards::utils::ownable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice!
We might want to have type of utils such as access utils or token utils. Also, we can provide some documents under the utils. So, I think we can go one step further and have
standards::utils::access::ownable
crates/miden-standards/asm/standards/faucets/network_fungible.masm
Outdated
Show resolved
Hide resolved
|
@mmagician can you also take a look! ? :) |
I did a full refactor in branch openzeppelin/ownable_dos - moved all ownership logic inline to network_fungible.masm (no external module calls). Same result: set_item fails with "storage slot not found". Root cause: When a note script calls an account procedure via call, the kernel's authenticate_account_origin sees the note script hash as the caller, not an account procedure. This blocks all storage writes from note scripts, regardless of where the code is defined. What works: Owner verification for minting (read-only). What doesn't work: transfer_ownership (requires write). This appears to be a fundamental Miden limitation. @mmagician is there a way to workaround it ? |
chore: restore stack manipulation
| # | ||
| # See the `NetworkFungibleFaucet` Rust type's documentation for more details. | ||
|
|
||
| use miden::standards::utils::access::ownable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: since we are using the fully-qualified path for exports, we shouldn't need to add this import.
An alternative could be to do something like this:
use miden::standards::utils::access::ownable
pub use ownable::transfer_ownership
But we kind of been using fully-qualified paths in such situations - so, probably should stick with it.
…dures in network fungible faucet
42c387a to
176eb62
Compare
crates/miden-standards/asm/standards/faucets/network_fungible.masm
Outdated
Show resolved
Hide resolved
crates/miden-standards/asm/standards/faucets/network_fungible.masm
Outdated
Show resolved
Hide resolved
| #! | ||
| #! Inputs: [pad(16)] | ||
| #! Outputs: [pad(16)] | ||
| pub proc renounce_ownership |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feature is also included in the Ownable module for OpenZeppelin Solidity contracts: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/1e5b89110853663f4d7f83c867b3945854f40a76/contracts/access/Ownable.sol#L76C14-L76C31
The idea of using it for decentralization of the network at some point. @mmagician was considering this might be confusing for user.
It states that:
renounceOwnership for the owner to relinquish this administrative privilege, a common pattern after an initial stage with centralized administration is over.
Let's see if there is any comment related to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's alright to keep renounce_ownership 👍🏼 and give developers an early path to decentralization
| # => [pad(16)] | ||
|
|
||
| # ---- Write zero AccountId to storage ---- | ||
| push.0.0.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can model zero address not to be usable anyone, and we can name it AddressZero in the kernel. @mmagician
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can assume that nobody will be able to come up with address that is all zeros (if they can, then collision-resistance of the hash function is broken, and we have bigger problems).
We could also add this in the kernel - but not sure where yet (and maybe that should be a separate issue).
For now, I would create a constant in this file and then just do something like push.ZERO_ADDRESS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also add this in the kernel - but not sure where yet
I don't think there would be any benefits to doing this in the kernel. In Ethereum this is not defined at the protocol level, but rather it's assumed (by preimage resistance) that no one will have the key to that account.
bobbinth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I some comments inline - mostly minor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then namespace feels a bit off for me. Do we really need the utils namespace? Maybe this could be just miden::standards::access::ownable
| # => [pad(16)] | ||
|
|
||
| # ---- Write zero AccountId to storage ---- | ||
| push.0.0.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can assume that nobody will be able to come up with address that is all zeros (if they can, then collision-resistance of the hash function is broken, and we have bigger problems).
We could also add this in the kernel - but not sure where yet (and maybe that should be a separate issue).
For now, I would create a constant in this file and then just do something like push.ZERO_ADDRESS.
crates/miden-standards/asm/standards/faucets/network_fungible.masm
Outdated
Show resolved
Hide resolved
crates/miden-standards/asm/standards/faucets/network_fungible.masm
Outdated
Show resolved
Hide resolved
crates/miden-standards/asm/standards/faucets/network_fungible.masm
Outdated
Show resolved
Hide resolved
…and update references in network fungible faucet
|
@bobbinth @onurinanc @mmagician I changed/solved the issues/suggestions highlighted here in the PR. |

This PR creates a new reusable
ownablemodule that centralizes ownership management functionality for account components.The network fungible faucet has been refactored to use this module instead of implementing ownership logic inline, replacing the local
is_ownerprocedure withownable::check_ownerand updating the storage slot to use the standardownable::owner_configslot name. Thenetwork_fungible_faucetcomponent now also exportstransfer_ownershipfrom the ownable module.This change improves code reusability and maintainability by standardizing ownership logic across components. New tests have been added to verify ownership checks work correctly, and all existing tests continue to pass.
OpenZeppelin/miden-confidential-contracts#34 ( regarding this discussion ).