docs: Module account address documentation #22289
Conversation
julienrbrt
changed the title
📝 WalkthroughWalkthroughThe pull request introduces significant updates to the Changes
Assessment against linked issues
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
Documentation and Community
|
|
|
||
| # Module Accounts | ||
|
|
||
| ## Address Generation |
There was a problem hiding this comment.
That's a bit short for it to be in a dedicated page maybe. Having it in building modules is as well weird. Maybe an advanced concept?
|
Don't we have a page on accounts? If so let's add that content there maybe. Having a dedicated page is still weird. |
![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
🧹 Outside diff range and nitpick comments (3)
docs/learn/beginner/03-accounts.md (3)
Line range hint
115-156: Add examples to clarify public key formatsThe Public Keys section would benefit from concrete examples showing:
- Actual compressed public key format
- JSON representation of different key types
- How to work with the
cryptotypes.PubKeyinterfaceConsider adding examples like:
This prefix is followed by the `x`-coordinate. + +For example, a secp256k1 public key might look like: +```text +// Compressed public key (hex) +0x02a1633cafcc01ebfb6d78e39f687a1f0995c62fc95f51ead10a02ee0be551b5dc + +// JSON representation +{ + "@type": "/cosmos.crypto.secp256k1.PubKey", + "key": "AiFjPK/MAev7bXjjn2h6H5ZVxi/JX1Hq0QoC7gvlUbXc" +} +```
Line range hint
157-385: Enhance the secp256r1 implementation exampleThe secp256r1 implementation example would benefit from:
- Error handling best practices
- Comments explaining the cryptographic operations
- Security considerations
Consider enhancing the example:
// NewPrivKeyFromSecret creates a private key derived for the secret number // represented in big-endian. The `secret` must be a valid ECDSA field element. func NewPrivKeyFromSecret(secret []byte) (*PrivKey, error) { + // Validate input + if len(secret) == 0 { + return nil, errorsmod.Wrap(errors.ErrInvalidRequest, "secret cannot be empty") + } + var d = new(big.Int).SetBytes(secret) if d.Cmp(secp256r1.Params().N) >= 1 { return nil, errorsmod.Wrap(errors.ErrInvalidRequest, "secret not in the curve base field") } + + // Initialize private key with proper security parameters sk := new(ecdsa.PrivKey) return &PrivKey{&ecdsaSK{*sk}}, nil }Also consider adding a security note:
+// Security Note: This implementation assumes proper entropy in the input secret. +// In production, always ensure the secret comes from a cryptographically secure +// random number generator. func (s secp256r1Algo) Generate() GenerateFn {
107-113: Update code references to use tagged versionsThe code references use specific commit hashes which may become outdated. Consider:
- Using tagged version references instead of commit hashes
- Adding more context around code examples
- Ensuring links are versioned consistently
Update the references to use tagged versions:
-https://github.com/cosmos/cosmos-sdk/blob/3a03804c148d0da8d6df1ad839b08c50f6896fa1/simapp/app.go#L130-L141 +https://github.com/cosmos/cosmos-sdk/blob/v0.52.0-beta.2/simapp/app.go#L130-L141
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
docs/learn/beginner/03-accounts.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/learn/beginner/03-accounts.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
docs/learn/beginner/03-accounts.md
Outdated
|
|
||
| ### Module Accounts | ||
|
|
||
| #### Address Generation | ||
|
|
||
| Module account addresses are generated deterministically from the module name, as defined in [ADR-028](../../architecture/adr-028-public-key-addresses.md) | ||
|
|
||
| Definition of account permissions is done during the app initialization. | ||
|
|
||
| ```go reference | ||
| https://github.com/cosmos/cosmos-sdk/blob/3a03804c148d0da8d6df1ad839b08c50f6896fa1/simapp/app.go#L130-L141 | ||
| ``` | ||
|
|
||
| ```go reference | ||
| https://github.com/cosmos/cosmos-sdk/blob/3a03804c148d0da8d6df1ad839b08c50f6896fa1/simapp/app.go#L328 | ||
| ``` | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the Module Accounts section with more details
The Module Accounts section needs improvement in the following areas:
- Add explanation of what module accounts are and their purpose
- Provide specific examples of module account address generation
- Explain the permissions system and its implications
- Add context for the code references to help readers understand their purpose
Consider adding content like this before the code references:
### Module Accounts
+
+Module accounts are special-purpose accounts for modules to perform specific operations. Unlike regular accounts, they are controlled by the module's logic rather than private keys.
+
#### Address Generation
-Module account addresses are generated deterministically from the module name, as defined in [ADR-028](../../architecture/adr-028-public-key-addresses.md)
+Module account addresses are generated deterministically from the module name, as defined in [ADR-028](../../architecture/adr-028-public-key-addresses.md). For example, if a module is named "mint", its address would be derived using:
+```go
+authtypes.NewModuleAddress("mint")
+```
+
+This ensures that module accounts have consistent addresses across all chains using the same module name.
-Definition of account permissions is done during the app initialization.
+Module accounts can be granted specific permissions during app initialization. These permissions control what operations the module can perform, such as minting tokens or sending tokens to accounts.
| @@ -95,6 +95,23 @@ https://github.com/cosmos/cosmos-sdk/blob/v0.52.0-beta.2/codec/address/bech32_co | |||
| | Validator Operator | cosmosvaloper | | |||
| | Consensus Nodes | cosmosvalcons | | |||
|
|
|||
|
|
|||
| ### Module Accounts | |||
|
|
|||
There was a problem hiding this comment.
let's have an intro on module accounts as well
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
docs/learn/beginner/03-accounts.md (3)
101-102: Enhance the module accounts introduction with more technical details.While the introduction provides a good overview, it would be beneficial to add:
- Explanation of how module accounts differ from regular accounts in terms of capabilities
- Common use cases and examples of module accounts (e.g., fee collector, bonded tokens pool)
- Security implications of module accounts
Line range hint
118-157: Add practical examples of public key usage.The section would benefit from:
- Code examples showing how to work with different public key types
- Examples of public key serialization and deserialization
- Common use cases and best practices
Example addition:
Public Keys are not used to reference accounts (or users) and in general are not used when composing transaction messages (with a few exceptions: `MsgCreateValidator`, `Validator` and `Multisig` messages). + +Here's an example of working with a secp256k1 public key: + +```go +// Example of creating and serializing a public key +pubKey := secp256k1.GenPrivKey().PubKey() +pubKeyBytes := pubKey.Bytes() + +// Example of deserializing a public key +recoveredPubKey := secp256k1.PubKey{ + Key: pubKeyBytes, +} +```
Line range hint
159-385: Add security considerations for key implementation.The secp256r1 implementation example should include:
- Security considerations when implementing new key types
- Best practices for key generation and storage
- Common pitfalls to avoid
Example addition:
Finally, the algo must be added to the list of [supported algos](https://github.com/cosmos/cosmos-sdk/blob/v0.52.0-beta.2/crypto/keyring/keyring.go#L209) by the keyring. + +### Security Considerations + +When implementing a new key type: +- Ensure the key generation uses a cryptographically secure random number generator +- Implement proper key encryption at rest +- Validate all inputs to prevent potential cryptographic weaknesses +- Consider side-channel attacks in the implementation +- Follow the principle of least privilege when exposing key operations
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
docs/learn/beginner/03-accounts.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/learn/beginner/03-accounts.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
| Module account addresses are generated deterministically from the module name, as defined in [ADR-028](../../architecture/adr-028-public-key-addresses.md) | ||
|
|
||
| Definition of account permissions is done during the app initialization. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add practical examples of module account address generation.
The documentation should include:
- Concrete examples of how module account addresses are generated
- The specific format and structure of the generated addresses
- Example code showing how to generate and validate module account addresses
Example addition:
Module account addresses are generated deterministically from the module name, as defined in [ADR-028](../../architecture/adr-028-public-key-addresses.md)
+
+For example, to generate a module account address for the "mint" module:
+```go
+mintModuleAddr := authtypes.NewModuleAddress("mint")
+fmt.Printf("Mint module address: %s\n", mintModuleAddr)
+```
+
+Common module account names include:
+- "fee_collector": Collects transaction fees
+- "distribution": Handles reward distribution
+- "bonded_tokens_pool": Manages staked tokens
+- "not_bonded_tokens_pool": Manages unstaked tokens| ```go reference | ||
| https://github.com/cosmos/cosmos-sdk/blob/3a03804c148d0da8d6df1ad839b08c50f6896fa1/simapp/app.go#L130-L141 | ||
| ``` | ||
|
|
||
| ```go reference | ||
| https://github.com/cosmos/cosmos-sdk/blob/3a03804c148d0da8d6df1ad839b08c50f6896fa1/simapp/app.go#L328 | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add context to code references.
The code references need explanatory text to help readers understand:
- What these code snippets demonstrate
- How they relate to module account initialization
- The significance of the permissions being set
Example addition:
+The following code demonstrates how module accounts are initialized in a Cosmos SDK application:
+
```go reference
https://github.com/cosmos/cosmos-sdk/blob/3a03804c148d0da8d6df1ad839b08c50f6896fa1/simapp/app.go#L130-L141+This initialization process:
+1. Creates module accounts with specific names
+2. Assigns appropriate permissions to each module account
+3. Registers the accounts in the auth keeper
<!-- This is an auto-generated comment by CodeRabbit -->
(cherry picked from commit 3ba4661)
* main: (24 commits) build(deps): upgrade to iavl@v1.3.1 (#22436) docs: Update tendermint validators query pagination documentation (#22412) refactor(client/v2): offchain uses client/v2/factory (#22344) feat: wire new handlers to grpc (#22333) fix(x/group): proper address rendering in error (#22425) refactor(serevr/v2/cometbft): update `RegisterQueryHandlers` and GRPC queries (#22403) docs: update ADR 59 (#22423) build(deps): Bump github.com/fsnotify/fsnotify from 1.7.0 to 1.8.0 in /tools/cosmovisor (#22407) docs: Module account address documentation (#22289) feat(baseapp): add per message telemetry (#22175) docs: Update Twitter Links to X in Documentation (#22408) docs: redirect the remote generation page (#22404) refactor(serverv2): remove unused interface methods, honuor context (#22394) fix(server/v2): return ErrHelp (#22399) feat(indexer): implement `schema.HasModuleCodec` interface in the `bank` module (#22349) refactor(math): refactor ApproxRoot for readality (#22263) docs: fix KWallet Handbook (#22395) feat(client/v2): broadcast logic (#22282) refactor(server/v2): eager config loading (#22267) test(system): check feePayers signature (#22389) ...
Description
Closes: #9916
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
secp256r1algorithm.