Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: use contract configs #731

Merged
merged 27 commits into from
Jul 26, 2024
Merged

feat: use contract configs #731

merged 27 commits into from
Jul 26, 2024

Conversation

ChaoticTempest
Copy link
Member

This make use of contract configs in our node.

  • --override-config parameter for partial overrides over the config in the contract
  • we have less environment variables needed now, alongside less CLI arguments too

@ChaoticTempest
Renamed chain-signature MPC_RECOVERY_* env vars to MPC_*
ce04dcf
@ChaoticTempest
Rename mpc-recovery-node binary to mpc-node
c02b47e
@ChaoticTempest
Rename some forgotten binary names
cf59bf2
@ChaoticTempest
Made update methods us MpcContractError
c02663a
@ChaoticTempest
Made proposed updates more space efficient
54044b8
@ChaoticTempest
Added bytes_used for config and code updates
6381121
@ChaoticTempest
Removed unnecessary near-rng crate
a98dd91
@ChaoticTempest
Added deposit to propose_update
00847f6
@ChaoticTempest
Make config dynamic for easier changes
a5d4cc4
@ChaoticTempest
Added proper borsh serialization and payable
31342cc
@ChaoticTempest
Use to_vec instead for config borsh ser
7270810
@ChaoticTempest
Update deposit cost
41340b6
@ChaoticTempest
Clippy
3858a82
@ChaoticTempest
Added propose_update refund diff and migrate
b213d8e
@ChaoticTempest
Added additional test case for a contract that fails to migrate
cf17f8a
@ChaoticTempest
Update INVALID contract path
81af3fb
@ChaoticTempest
Made config module
8dab958
@ChaoticTempest
Made config an option that can be initialized from contract
2190faf
@ChaoticTempest
Made node use contract config with LocalConfig
644be9b
@ChaoticTempest
Added --override-config for partial overrides
cf320f7
@ChaoticTempest
Added SignatureConfig
eb84b36
@ChaoticTempest
Use timeouts from contract configs
a08817e
@ChaoticTempest
Removed no longer needed env variables from tf
4d7931f
@ChaoticTempest
Moved GC timeout to contract config
d9b358b
@ChaoticTempest
Merge branch 'develop' of github.com:near/mpc-recovery into phuong/fe…
464887e 
…at/use-contract-configs
@ChaoticTempest ChaoticTempest requested review from volovyks and ppca July 25, 2024 09:52
@volovyks
Copy link
Collaborator

@ChaoticTempest I've fixed a conflict and contract tests, please, check the latest commits.
Part of the contract update test is commented. We are hitting transaction size.

volovyks
volovyks reviewed Jul 25, 2024
View reviewed changes
chain-signatures/contract/tests/tests.rs
@@ -404,7 +404,7 @@ async fn test_contract_propose_update() {
dbg!(contract.id());

test_propose_update_config(&contract, &accounts).await;
test_propose_update_contract(&contract, &accounts).await;
// _test_propose_update_contract(&contract, &accounts).await;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The contract is too big.

volovyks
volovyks approved these changes Jul 25, 2024
View reviewed changes
Copy link
Collaborator

@volovyks volovyks left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!
Not sure what is going on with cargo check, it is all good on my machine and GCP VM.

chain-signatures/node/src/protocol/mod.rs Show resolved Hide resolved
ailisp
ailisp reviewed Jul 26, 2024
View reviewed changes
chain-signatures/node/src/protocol/mod.rs
Err(e) => {
tracing::error!("could not fetch contract's config: {e}");
tokio::time::sleep(Duration::from_secs(1)).await;
continue;
Copy link
Member

Choose a reason for hiding this comment

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

Is one second too frequent, config is not very frequently changed.

And above rpc_client::fetch_mpc_config.await blocks the main tokio thread, slow near rpc could slowdown protocol state handle(). I would suggest merge config with state call, doing it less frequently, or fetch it in a background tokio thread, without await, and run read from the fetched config written by the background thread, it could be 1s old but next MpcSign will get it

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah 1s might be too frequent actually for configs that rarely if ever change. We can probably have a background task that fetches the config every 10 minutes or so as to not block this thread

ailisp
ailisp approved these changes Jul 26, 2024
View reviewed changes
Copy link
Member

@ailisp ailisp left a comment

Choose a reason for hiding this comment

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

Nice improvement! Although we didn't test the new config is applied in an integration test, I think the code looks right, config is refreshed and pass as function arguments instead of state to protocol modules, they'll always get latest config.

@volovyks
Copy link
Collaborator

@ChaoticTempest I'm merging this to avoid conflicts. Let's address all the comments in a follow-up PR.

@volovyks volovyks merged commit 5762932 into develop Jul 26, 2024
3 checks passed
@volovyks volovyks deleted the phuong/feat/use-contract-configs branch July 26, 2024 11:58
@github-actions GitHub Actions
Copy link

Terraform Feature Environment Destroy (dev-731)

Terraform Initialization ⚙️success

Terraform Destroy success

Show Destroy Plan 


No changes. No objects need to be destroyed.

Either you have not created any objects yet or the existing objects were
already deleted outside of Terraform.

Destroy complete! Resources: 0 destroyed.

Pusher: @volovyks, Action: pull_request, Working Directory: ``, Workflow: Terraform Feature Env (Destroy)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ailisp ailisp ailisp approved these changes

@volovyks volovyks volovyks approved these changes

@ppca ppca Awaiting requested review from ppca

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ChaoticTempest @volovyks @ailisp