Set stream.getminiblockmaxpagesize onchain setting#4706
Conversation
begmaroman
requested review from
bas-vk,
giuseppecrj,
jterzis,
pfives,
sergekh2,
shuhuiluo and
texuf
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryStandardizes the setting key to
Written by Cursor Bugbot for commit 59b13fa. This will update automatically on new commits. Configure here. |
📝 WalkthroughWalkthroughStandardizes the miniblocks max page-size config key to lowercase, sets the Go default to 200, adds a Solidity keccak256 constant for the key, and adds a Foundry interaction script to set the on-chain config; a doc comment was updated accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/node/events/remote_provider.go (1)
32-36: Doc key matches config; tiny wording nitComment correctly points to
stream.getminiblocksmaxpagesize. If you touch this again, considerRange is limited to chain config setting ...for slightly clearer phrasing.packages/contracts/scripts/interactions/InteractSetGetMiniblocksMaxPageSize.s.sol (1)
14-26: Interaction logic is straightforward; keep 200 in sync with node defaultThe script cleanly sets the new config via
STREAM_GET_MINIBLOCKS_MAX_PAGE_SIZEandabi.encode(uint64(200)), matching the Go default. Just be aware this 200 is now duplicated here and inDefaultOnChainSettings; if the limit ever changes, both will need updating.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
core/env/local/local_dev/on_chain.csvis excluded by!**/*.csv
📒 Files selected for processing (4)
core/node/crypto/config.go(3 hunks)core/node/events/remote_provider.go(1 hunks)packages/contracts/scripts/interactions/InteractSetGetMiniblocksMaxPageSize.s.sol(1 hunks)packages/contracts/scripts/interactions/helpers/RiverConfigValues.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format all Go files using./fmt.shscript from the/coredirectory
Run the linter using./lint.shscript from the/coredirectory for Go code
Use structured logging with zap for Go backend code
Implement proper error handling with RiverError types in Go code
Implement proper context cancellation for graceful shutdowns in Go code
Files:
core/node/crypto/config.gocore/node/events/remote_provider.go
core/**/*.go
📄 CodeRabbit inference engine (core/AGENTS.md)
core/**/*.go: Format all modified Go files by running./fmt.shfrom the/coredirectory before committing
Run the linter via./lint.shfrom the/coredirectory to check code quality using golangci-lint and staticcheck, and fix any issues before committing
UseRiverErrortype fromnode/base/error.gofor structured errors
UseRiverErrorWithBaseto construct a river error derived from an error of arbitrary or unknown type
Wrap blockchain errors with context usingcrypto/utilities
Log errors with structured fields using zap logger
Use structured JSON logging via zap with custom extensions innode/logging/for log output
Use PostgreSQL connection pooling via pgx/v5 for database connections
Files:
core/node/crypto/config.gocore/node/events/remote_provider.go
packages/contracts/**/*.sol
📄 CodeRabbit inference engine (packages/contracts/.cursor/rules/contracts.mdc)
packages/contracts/**/*.sol: All contracts must follow the Diamond Pattern for upgradeability
Use explicit visibility modifiers for all functions and state variables
Implement proper access control using modifiers
Follow gas optimization best practices
Use events for important state changes
Implement proper input validation
Follow semantic versioning for contract upgrades
Use calldata in external function argument definitions when possible
Prefer clear/descriptive names over excessive comments
Library functions should be pure in the functional programming sense (no side effects)
Use diamond storage pattern for all state variables
Namespace storage slots to avoid collisions
Use efficient data structures for gas
Use proper mapping patterns for complex data
Use strict typing for all variables/parameters
Implement interface inheritance
Use enums for predefined sets
Use structs for complex types
Follow ABI encoding/decoding best practices
Implement granular permission checks
Use role-based access control
Validate cross-chain entitlements
Implement permission override mechanisms
Use NatSpec for all public/external functions
Document complex logic/algorithms
Keep inline comments concise
Document upgrade procedures
Use efficient data structures, pack storage
Implement batch operations where possible
Minimize storage reads/writes
Cache frequently accessed storage
Use unchecked blocks where safe
Usemsg.senderdirectly
Follow CEI (Checks-Effects-Interactions) pattern
Use reentrancy guards
Validate all inputs
Use safe math
Implement access control
Usefor (uint256 i; i < length; ++i)for loops
Follow upgrade best practices
packages/contracts/**/*.sol: Minimize complexity in software structure, prioritizing obviousness and ease of modification in Solidity contracts and facets
Invest time in design upfront rather than quick tactical fixes; prioritize long-term structure over short-term convenience when adding new facets or features
Design code to be obvious: developers ...
Files:
packages/contracts/scripts/interactions/InteractSetGetMiniblocksMaxPageSize.s.solpackages/contracts/scripts/interactions/helpers/RiverConfigValues.sol
**/*.{ts,tsx,js,jsx,yaml,yml,sol,json,md}
📄 CodeRabbit inference engine (AGENTS.md)
All non-Go files (TypeScript, JavaScript, YAML, Solidity) must pass Prettier formatting by running
bun run prettier:fix
Files:
packages/contracts/scripts/interactions/InteractSetGetMiniblocksMaxPageSize.s.solpackages/contracts/scripts/interactions/helpers/RiverConfigValues.sol
🧠 Learnings (2)
📚 Learning: 2025-11-25T08:48:08.084Z
Learnt from: CR
Repo: towns-protocol/towns PR: 0
File: core/node/storage/AGENTS.md:0-0
Timestamp: 2025-11-25T08:48:08.084Z
Learning: Applies to core/node/storage/storage/**/*.go : Enforce miniblock requirement constraint: if a stream exists in the `es` (event streams) table, it MUST have at least one miniblock; empty streams are not allowed
Applied to files:
core/node/crypto/config.go
📚 Learning: 2025-11-25T08:48:08.084Z
Learnt from: CR
Repo: towns-protocol/towns PR: 0
File: core/node/storage/AGENTS.md:0-0
Timestamp: 2025-11-25T08:48:08.084Z
Learning: Applies to core/node/storage/storage/**/*.go : Enforce minipool generation sequencing constraint: the minipool generation number MUST always be set to the last miniblock number + 1
Applied to files:
core/node/crypto/config.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: XChain_Integration
- GitHub Check: Build_Anvil_Docker
🔇 Additional comments (3)
packages/contracts/scripts/interactions/helpers/RiverConfigValues.sol (1)
26-27: New config key constant matches Go sideThe
STREAM_GET_MINIBLOCKS_MAX_PAGE_SIZEhash over"stream.getminiblocksmaxpagesize"is aligned with the Go config key and lowercase hashing convention; this should decode correctly on the node side.packages/contracts/scripts/interactions/InteractSetGetMiniblocksMaxPageSize.s.sol (1)
1-2: Solidity pragma differs from stated baselineThis script uses
pragma solidity ^0.8.24;while guidelines reference^0.8.23. That’s fine if your toolchain/compiler is already on 0.8.24, but worth confirming or aligning with the rest of the contracts for consistency.core/node/crypto/config.go (1)
47-47: Config key rename is consistent and completeLowercase key
"stream.getminiblocksmaxpagesize"correctly applied to the constant, struct tag, and default value (200). No lingering references to the old camelCase variant.
|
There is already a sub command that allows you to set on-chain settings. For this you can run: |
| function __interact(address deployer) internal override { | ||
| address riverRegistry = getDeployment("riverRegistry"); | ||
|
|
||
| uint64 value = 200; |
There was a problem hiding this comment.
We need to be careful with such a large number. Currently there are streams that have a large range of miniblocks with each miniblock of 10MB in size. If something requests 200 of these miniblocks it will result in 2GB DB resultset which is problematic. This was the result of temporarily problems so after these huge ranges are trimmed this should not be a problem anymore.
There was a problem hiding this comment.
I think we should set the value anyway and 200 is good starting point. For large streams, there will be an alternative solution. It could be using getStreamEx endpoint or so.
As far as I remember, we should do it via solidity scripts and pass it through the governance so the script is required. |
2 participants
No description provided.