-
Notifications
You must be signed in to change notification settings - Fork 8
pax/chain predeploy #384
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: develop
Are you sure you want to change the base?
pax/chain predeploy #384
Conversation
nick1udwig
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.
I like the direction you're going here. Added some comments. In general we need to make sure we make it clear to users How to use this feature without reading kit code. Right now it is not clear at all
E.g.:
- How to I acquire the contract code I need to deploy it on the fakechain?
- How do I learn the format of the file I need to use for the config file?
src/chain/mod.rs
Outdated
| .await?; | ||
|
|
||
| // Always try to load default config | ||
| let default_config = load_config(&PathBuf::from(DEFAULT_CONFIG_PATH))?; |
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.
instead of loading this from a path (which happens at run-time, where kit may be running in an arbitrary directory), you're going to want to include_str! it, which includes it at compile-time (and so guarantees you'll have access to it at run-time). See example usage
src/chain/mod.rs
Outdated
| (HYPERMAP_PROXY, include_str!("./bytecode/erc1967proxy.txt")), | ||
| (HYPERMAP, include_str!("./bytecode/hypermap.txt")), | ||
| ]; | ||
| const PREDEPLOY_CONTRACTS: &[(&str, &str)] = &[]; |
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.
no reason to keep these or the logic that uses them around if they are gone now
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, will remove all txt configs
|
|
||
| // Public function without custom config | ||
| #[instrument(level = "trace", skip_all)] | ||
| pub async fn start_chain( |
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 should remove this and rename start_chain_with_config to start_chain (and update the callers to pass in None
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.
agree, will do once we have working POC
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.
Ok lets update this now
| .action(ArgAction::Set) | ||
| .short('c') | ||
| .long("config") | ||
| .help("Path to contracts config file (TOML format)") |
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.
it isn't obvious from this helptext that what is happening is we are configuring additional contracts with the same form as the default. Update the helptext to be more descriptive here
|
|
||
| 3. Deploy missing contracts if needed | ||
|
|
||
| ## Troubleshooting |
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.
remove this section
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.
removed
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.
How to make the contract files from Solidity? Give examples in foundry and 1 other framework
In general this doc is great for what it covers, but it doesn't tell me how to go from some solidity smart contracts or transactions I want to run to stuff I can plug in here
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.
done
|
|
||
| // Public function without custom config | ||
| #[instrument(level = "trace", skip_all)] | ||
| pub async fn start_chain( |
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.
Ok lets update this now
| } | ||
|
|
||
| #[instrument(level = "trace", skip_all)] | ||
| pub async fn verify_contracts(port: u16, addresses: &ContractAddresses) -> Result<()> { |
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 need to either generalize this function or else rename it: right now all it does is verify hypermap
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.
done
src/chain/mod.rs
Outdated
| ) | ||
| .await?; | ||
|
|
||
| let default_config = load_default_config().ok(); |
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 should never fail. Don't .ok(), just ?
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.
done
src/chain/mod.rs
Outdated
|
|
||
| #[instrument(level = "trace", skip_all)] | ||
| async fn check_dot_os_tba(port: u16) -> Result<bool> { | ||
| let dot_os_tba = "0x9b3853358ede717fc7D4806cF75d7A4d4517A9C9"; |
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 should be a const defined at top of file
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.
Or better yet read in from config file
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.
Probably this function should be generalized and take the address as an argument
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.
done
src/chain/mod.rs
Outdated
| sleep(Duration::from_millis(200)).await; | ||
|
|
||
| // Calculate token ID from label (.os) | ||
| let token_id_hex = "0xdeeac81ae11b64e7cab86d089c306e5d223552a630f02633ce170d2786ff1bbd"; |
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.
there are lots of these magic numbers throughout the code. These should all either be pulled from the config or defined as const at the top of the file
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.
done
| Ok(tx_hash) => { | ||
| info!("Mint NFT transaction sent: {}", tx_hash); | ||
|
|
||
| sleep(Duration::from_millis(200)).await; |
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 need a more robust solution here. Ideally we would have a way to tell that we are ready to proceed by querying the chain. Worst case, we should sleep-retry until ready
src/chain/mod.rs
Outdated
| } | ||
| } | ||
| Err(e) => { | ||
| info!("Mint NFT transaction failed: {}", e); |
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.
Is this the proper logging level to use in this case?
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.
done
| ); | ||
|
|
||
| let client = Client::new(); | ||
| let _impersonator = AnvilImpersonator::new(port, &client, OWNER_ADDRESS).await?; |
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.
do we actually need to impersonate here since we execute_transaction with OWNER_ADDRESS as from in L709?
|
This breaks networking between fake nodes. I think what is going on is that everything gets re-deployed when you start a second fake node. This needs to be fixed before merging Look at this page for an example of how two boot two fake nodes: https://book.hyperware.ai/getting_started/quick_start.html On master, you can then do, e.g., from the second fakenode and it will properly message fake.os On this branch they cannot see each other, like I said, probably because the chain gets reset? Could also have broken if you changed how new fake nodes register on the chain when they boot |
|
Fixed running two fake nodes together on the same network but it also must be fixed on hyperdrive level, it has hardcoded dotOs address that is different from the kit one. Ideally we have to use hypermap.tbaOf(dotOstokenId) |
#301
configurable predeploy + custom deployed option