refactor(sdk): Remove NetworkArg + Make functions that accept Network methods of Network#1598
refactor(sdk): Remove NetworkArg + Make functions that accept Network methods of Network#1598
NetworkArg + Make functions that accept Network methods of Network#1598Conversation
PatStiles
changed the title
NetworkArg + Make functions that accept Network methods of Network
| } | ||
|
|
||
| impl Network { | ||
| pub fn get_batcher_payment_service_address(&self) -> ethers::types::H160 { |
There was a problem hiding this comment.
Nit: pass by copy:
| pub fn get_batcher_payment_service_address(&self) -> ethers::types::H160 { | |
| pub fn get_batcher_payment_service_address(self) -> ethers::types::H160 { |
There was a problem hiding this comment.
Now that Custom(String, String) it's no longer a good idea to pass by copy. That's also true for is_proof_verified.
| } | ||
| } | ||
|
|
||
| pub fn get_aligned_service_manager_address(&self) -> ethers::types::H160 { |
There was a problem hiding this comment.
Nit: pass by copy:
| pub fn get_aligned_service_manager_address(&self) -> ethers::types::H160 { | |
| pub fn get_aligned_service_manager_address(self) -> ethers::types::H160 { |
There was a problem hiding this comment.
- It would be better for NetworkArg, requiring clap in the SDK is a bit overkill
- There needs to be a way to use values that are not the default ones, so in case we lunch another network we can use it without changing the code (Either through a config where you load network names and addresses, or some different input, may need to think a bit this)
Maybe a |
| pub const BATCHER_PAYMENT_SERVICE_ADDRESS_MAINNET: &str = | ||
| "0xb0567184A52cB40956df6333510d6eF35B89C8de"; | ||
| /// AlignedServiceManager | ||
| pub const ALIGNED_SERVICE_MANAGER_DEVNET: &str = "0x1613beB3B2C4f22Ee086B2b38C1476A3cE7f78E8"; |
There was a problem hiding this comment.
Shouldn't it be 0x851356ae760d987E095750cCeb3bC6014560891C?
| pub const ALIGNED_SERVICE_MANAGER_HOLESKY: &str = "0x58F280BeBE9B34c9939C3C39e0890C81f163B623"; | ||
| pub const ALIGNED_SERVICE_MANAGER_HOLESKY_STAGE: &str = | ||
| "0x9C5231FC88059C086Ea95712d105A2026048c39B"; | ||
| pub const ALIGNED_SERVICE_MANAGER_HOLESKY_MAINNET: &str = |
There was a problem hiding this comment.
| pub const ALIGNED_SERVICE_MANAGER_HOLESKY_MAINNET: &str = | |
| pub const ALIGNED_SERVICE_MANAGER_MAINNET: &str = |
| } | ||
| let parts: Vec<&str> = s.split_whitespace().collect(); | ||
|
|
||
| if parts.len() == 3 { |
There was a problem hiding this comment.
We should also enforce custom to be parts[0].
JulianVentura
left a comment
JulianVentura
left a comment
There was a problem hiding this comment.
Should update documentation to add new custom network parameter on docs/3_guides/0_submitting_proofs.md
|
We should also update |
|
For now the deployment of a custom network will be complete via stating "custom BATCHER_PAYMENT_SERVICE ALIGNED_SERVICE_MANAGER" in quotes. This solution is not ideal and we will discuss and implement a better solution in a follow up pr. |
PatStiles
force-pushed
the
refactor/network_arg
branch
2 times, most recently
from
70b2e30 to
413aada
Compare
|
Closing in favor of #1730 |
7 participants
Important
Closed in favour of #1730
Refactor Network Arg
Description
Refactors the Network struct in the sdk to:
ValueEnumand thus remove the need forNetworkArgNetworkand return addresses to be methods ofNetwork.Type of change
Please delete options that are not relevant.
Checklist
testnet, everything else tostaging