Skip to content

Conversation

alysiahuggins
Copy link
Contributor

@alysiahuggins alysiahuggins commented Sep 2, 2025

Closes #<ISSUE_NUMBER>

This PR:

This PR does not:

Key places to review:

How to test this PR:

  • run the timeboost-contract tests cargo test -p timeboost-contract

@alysiahuggins alysiahuggins marked this pull request as ready for review September 2, 2025 22:35
@alysiahuggins alysiahuggins changed the title Contract docs Deployment Scripts via Forge & Contract docs Sep 3, 2025
Copy link
Contributor

@akonring akonring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great with contract docs 💯.
LGTM, but I'll let others chime in.


# Use environment variables with defaults
RPC_URL=${RPC_URL:-"http://localhost:8545"}
ACCOUNT_INDEX=${ACCOUNT_INDEX:-0}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ACCOUNT_INDEX=${ACCOUNT_INDEX:-0}
ACCOUNT_INDEX=${ACCOUNT_INDEX:-0}
MNEMONIC=${MNEMONIC:-"test test test test test test test test test test test junk"}

Copy link
Contributor

@alxiong alxiong Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls use attend year erase basket blind adapt stove broccoli isolate unveil acquire category as the default mnemonic,

@alysiahuggins need to fund this account

# default test wallet's private key
FAUCET_PRIVATE_KEY="0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80"

# Fund the manager account
cast send --value 1ether --private-key "$FAUCET_PRIVATE_KEY" "$MANAGER_ADDRESS"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. As long as we fund the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is now in a seperate repo, this concern is no longer here (as discussed during the standup)

Copy link
Contributor

@lukeiannucci lukeiannucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me! i have nothing to add here. Thanks!

@alxiong
Copy link
Contributor

alxiong commented Sep 7, 2025

should we first move this work to https://github.com/EspressoSystems/timeboost-contracts? @alysiahuggins @lukeiannucci ?

alxiong
alxiong previously requested changes Sep 7, 2025
Copy link
Contributor

@alxiong alxiong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @alysiahuggins, the documentation looks fine. but we already have deployment via rust binary timeboost-contract/src/binaries/deploy.rs and we tried to avoid .env as per Toralf's suggestion since we want to centralize our config in the config files (you can see test-configs/c0/node_0.toml for an example instead of having both these node config files and .env files.

i also added a bash script in scripts/test-contract-deploy to 1) spawn anvil; 2) deploy contracts; 3) register committee (by invoking the binary register.rs).

(special note is that for the test wallet, I didn't use test test ... junk mnemonic phrase, as i explained here)


update: I take back my comments on avoiding .env and forge script. ignore that comment there.

@lukeiannucci
Copy link
Contributor

should we first move this work to https://github.com/EspressoSystems/timeboost-contracts? @alysiahuggins @lukeiannucci ?

i was thinking since we already started leaving comments here, we could finish it here then port it over. but if want to move the discussion and changes there, i have no problem

@lukeiannucci
Copy link
Contributor

thanks @alysiahuggins, the documentation looks fine. but we already have deployment via rust binary timeboost-contract/src/binaries/deploy.rs and we tried to avoid .env as per Toralf's suggestion since we want to centralize our config in the config files (you can see test-configs/c0/node_0.toml for an example instead of having both these node config files and .env files.

i also added a bash script in scripts/test-contract-deploy to 1) spawn anvil; 2) deploy contracts; 3) register committee (by invoking the binary register.rs).

(special note is that for the test wallet, I didn't use test test ... junk mnemonic phrase, as i explained here)

i personally like the secondary script to deploy, as we are going to be moving this contracts folder to its own respective repository, and when I am working in the nitro repository setting up tests there may come a time where i will need this, but I do see your point im fine either way.

@alxiong
Copy link
Contributor

alxiong commented Sep 8, 2025

i was thinking since we already started leaving comments here, we could finish it here then port it over. but if want to move the discussion and changes there, i have no problem

agree. my suggestion is: continue the discussion and address all comments in the current PR.
after all comments are addressed, dump all changes into a git diff file and apply in the new repo.
(namely: git diff --cached > pr482.patch then in the new repo, create a new PR via git apply pr482.patch)

i personally like the secondary script to deploy, ...when I am working in the nitro repository setting up tests there may come a time where i will need this, but I do see your point im fine either way.

i think i see your point. the cyclic dep issue if the nitro repo requires timeboost's binary to deploy these contract.
I agree that having this forge script (and bash script) is good.

my only concern is drifting config (.env and test-configs, e.g. the mnemonic used in .env file differs from what we expect, then the deployed contract address in test-configs all need to be revised). But i guess we can detect that discrepancy during integration test running in the CI.

Copy link
Contributor

@alxiong alxiong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contract side looks good to me, @alysiahuggins you can follow #482 (comment) and migrate the changes to the new contract repo.

then can we rebase (completely rewrite the git history of this branch) and only leave changes made to timeboost-contract and timeboost-proto?
@lukeiannucci do you have better suggestion?

Copy link
Contributor

@twittner twittner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this we have multiple ways to deploy the contract, a Rust binary (deploy.rs) plus shell script and now also a forge script, shell script and .env file. My preference would be to consolidate this and have only one way to deploy, specifically I would remove deploy.rs and test-contract-deploy. Unless I am missing something the .env file and deploy.sh in here can also be removed and we can add the forge script command to the existing justfile.

export MANAGER_ADDRESS=$MANAGER_ADDRESS

# Build forge command
FORGE_CMD="forge script script/DeployKeyManager.s.sol:DeployKeyManager --rpc-url $RPC_URL --broadcast --mnemonics \"$MNEMONIC\" --mnemonic-indexes $ACCOUNT_INDEX"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure why we need an .env file and deploy.sh? Can we not just run this forge command directly, e.g. from our Justfile?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good q and we don't need both but the contracts directory is actually going to be moved to this new repo and it would be good to have standalone deployment scripts there (and so far that repo doesn't use a justfile)

alysiahuggins added a commit to EspressoSystems/timeboost-contracts that referenced this pull request Sep 9, 2025
alysiahuggins added a commit to EspressoSystems/timeboost-contracts that referenced this pull request Sep 9, 2025
* Port EspressoSystems/timeboost#482 into timeboost-contracts

* remove redudunct readme and linked to the correct readme location for the timeboost repo

* remove lock files in gitignore

* improves readme
@alysiahuggins alysiahuggins dismissed alxiong’s stale review September 9, 2025 21:48

no longer needed as discussed in chat and the standup

@alysiahuggins alysiahuggins merged commit 6164a97 into main Sep 9, 2025
8 checks passed
@alysiahuggins alysiahuggins deleted the contract-docs branch September 9, 2025 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants