feat(katana): add fact registry arg for init - #3158
Conversation
WalkthroughOhayo sensei! This update introduces a new optional argument, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant InitArgs
participant SettlementProvider
User->>CLI: Run `katana init` with/without --settlement-facts-registry-contract
CLI->>InitArgs: Parse arguments
InitArgs->>CLI: Return parsed args (with optional facts registry contract)
CLI->>SettlementProvider: Initialize with args
alt Custom settlement chain
SettlementProvider->>CLI: Require facts registry contract, error if missing
else Known chain (Mainnet/Sepolia)
alt Custom facts registry provided
SettlementProvider->>CLI: Use custom facts registry contract
else
SettlementProvider->>CLI: Use default facts registry contract
end
end
CLI->>User: Success or error message
Assessment against linked issues
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
🪧 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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3158 +/- ##
==========================================
+ Coverage 55.59% 55.65% +0.05%
==========================================
Files 443 443
Lines 62964 63053 +89
==========================================
+ Hits 35006 35089 +83
- Misses 27958 27964 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Related PR for this feature: dojoengine/dojo#3158.
glihm
left a comment
glihm
left a comment
There was a problem hiding this comment.
Thank you for the contribution here @cwkang1998!
Even if piltover's facts registry address can be changed easily there: https://github.com/keep-starknet-strange/piltover/blob/161cb3f66d256e4d1211c6b50e5d353afb713a3e/src/config/interface.cairo#L77 this addition could make the deployment for testing easier using mocked facts registry straight away from Katana.
Any additional thoughts @kariy?
2a9723d to
5404e0b
Compare
![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 (1)
bin/katana/src/cli/init/mod.rs (1)
159-185: Clean implementation of the facts registry options, sensei!The code elegantly handles different settlement chains with proper conditional logic for the facts registry. For predefined chains (Mainnet and Sepolia), it uses the provided registry if available, otherwise falls back to defaults. For custom chains, it correctly requires the registry address, as agreed in previous discussions.
However, there's some code duplication between the Mainnet and Sepolia cases that could be refactored.
let settlement_provider_result = match settlement_chain { SettlementChain::Mainnet => { - let provider = SettlementChainProvider::sn_mainnet(); - Ok(match self.settlement_facts_registry_contract { - Some(fact_registry) => { - SettlementChainProvider::new(provider.url().clone(), *fact_registry) - } - None => provider, - }) + create_settlement_provider(SettlementChainProvider::sn_mainnet(), self.settlement_facts_registry_contract) } SettlementChain::Sepolia => { - let provider = SettlementChainProvider::sn_sepolia(); - Ok(match self.settlement_facts_registry_contract { - Some(fact_registry) => { - SettlementChainProvider::new(provider.url().clone(), *fact_registry) - } - None => provider, - }) + create_settlement_provider(SettlementChainProvider::sn_sepolia(), self.settlement_facts_registry_contract) } #[cfg(feature = "init-custom-settlement-chain")] SettlementChain::Custom(url) => match self.settlement_facts_registry_contract { Some(fact_registry) => Ok(SettlementChainProvider::new(url, *fact_registry)), None => Err(anyhow::anyhow!( "Fact registry contract address is required for custom settlement chain" )), }, };And add this helper function:
fn create_settlement_provider( provider: SettlementChainProvider, facts_registry: Option<ContractAddress>, ) -> Result<SettlementChainProvider, anyhow::Error> { Ok(match facts_registry { Some(fact_registry) => SettlementChainProvider::new(provider.url().clone(), *fact_registry), None => provider, }) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (6)
crates/dojo/core-cairo-test/Scarb.lockis excluded by!**/*.lockcrates/torii/types-test/Scarb.lockis excluded by!**/*.lockexamples/simple/Scarb.lockis excluded by!**/*.lockexamples/spawn-and-move/Scarb.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (2)
bin/katana/src/cli/init/mod.rs(3 hunks)crates/katana/cli/src/options.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/katana/cli/src/options.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
bin/katana/src/cli/init/mod.rs (5)
71-75: Ohayo! New CLI argument for facts registry looks great!This CLI argument addition allows specifying a custom facts registry contract, enhancing flexibility for different settlement configurations. The documentation is clear, and the arg dependencies are properly set up.
187-190: Nice error handling pattern, sensei!The error handling is clean and propagates the errors properly in the context of the CLI arguments. Using the early return pattern with
Some(Err(e))ensures that configuration errors don't get lost.
428-454: Excellent test for CLI argument parsing!This test ensures the new facts registry CLI argument is correctly parsed and stored in the
InitArgsstruct. The test provides good coverage for the happy path with all required arguments.
456-498: Good negative test case for required settlement arguments!This test verifies that the CLI correctly enforces the required settlement arguments when using the facts registry argument. The test is comprehensive and checks for all the expected error messages.
500-529: Perfect error validation for custom settlement chains!The test properly verifies that custom settlement chains require a facts registry contract address to be provided, validating both the error condition and the exact error message. Using an async test is appropriate since
configure_from_argsis an async method.This test aligns with the discussion in the previous review comments about requiring the facts registry for custom settlement chains.
kariy
left a comment
kariy
left a comment
There was a problem hiding this comment.
I'm retracting what I said in my last review. But I guess I wasn't clear enough anyway.
My assumption was that we don't want to allow setting arbitrary facts registry contract when we're settling on 'known' chain (i.e., mainnet and sepolia) because saya (at least for now) have a dependency on the Atlantic service. Hence, why I hardcoded the facts registries that Atlantic settles the proves to:
dojo/bin/katana/src/cli/init/settlement.rs
Lines 37 to 45 in a1a8dec
So, the latest changes you've made isn't exactly different from the last one in terms of the semantics.
But from I've seen during the development of saya, there were some cases where we were mocking some parts of the whole katana -> saya proving pipeline (iirc we're also mocking the facts registry at some point). So, it'd be convenient to set a different facts registry in which case having a setter function for it make sense.
So, I took the liberty to add back the setter function.
Got it, sorry for the misunderstanding. Will do a rebase and see if there's anything else I am missing. |
This PR adds a new optional argument `--settlement-facts-registry-contract` to `katana init` which allows passing a custom fact registry contract. Closes dojoengine#3034
136256a to
400cdaf
Compare
* feat(katana): add fact registry arg for `init` This PR adds a new optional argument `--settlement-facts-registry-contract` to `katana init` which allows passing a custom fact registry contract. Closes #3034 * fix: remove fact registry setter * feat: error in custom init w/ no fact registry * chore: refactor according to ai reviews * use setter function * fix test * chore: remove unneccessary changes --------- Co-authored-by: Ammar Arif <evergreenkary@gmail.com>
* feat(katana): add fact registry arg for `init` This PR adds a new optional argument `--settlement-facts-registry-contract` to `katana init` which allows passing a custom fact registry contract. Closes #3034 * fix: remove fact registry setter * feat: error in custom init w/ no fact registry * chore: refactor according to ai reviews * use setter function * fix test * chore: remove unneccessary changes --------- Co-authored-by: Ammar Arif <evergreenkary@gmail.com>
3 participants
Description
This PR adds a new optional argument
--settlement-facts-registry-contracttokatana initwhich allows passing a custom fact registry contract.Related issue
Fixes #3034
Tests
Added to documentation?
initbook#404Checklist
scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit