Skip to content

Commit

Permalink
[release-builder] Fix increase lockup
Browse files Browse the repository at this point in the history
The previous PR was converting the private key arg to string incorrectly...

ED25519 key type has a very silly to_string that returns debug output

I suppose this is to prevent dumping the key, but really i think it should just
not have a to_string and instead only have a debug method that does the same
thing, and a very explicit to hex method so that you dont accidentally dump these.

Test Plan: framework upgrade test succeeds, or at least doesnt fail on this
error (there is still some underlying flakiness in the forge test itself)
  • Loading branch information
perryjrandall authored and Zekun Wang committed Oct 13, 2023
1 parent d132ee7 commit 3d5db50
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 28 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions aptos-move/aptos-release-builder/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ aptos-crypto = { workspace = true }
aptos-framework = { workspace = true }
aptos-gas-schedule-updator = { workspace = true }
aptos-genesis = { workspace = true }
aptos-keygen = { workspace = true }
aptos-rest-client = { workspace = true }
aptos-temppath = { workspace = true }
aptos-types = { workspace = true }
Expand Down
99 changes: 71 additions & 28 deletions aptos-move/aptos-release-builder/src/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,16 @@ impl NetworkConfig {
})
}

// ED25519 Private keys have a very silly to_string that returns what looks
// like a debug output:
// <elided secret for Ed25519PrivateKey>
// Which is almost never what you want unless you're debugging...
// Usually you want the hex encoded version, although you wanna be careful
// you're not accidentally leaking private keys, but thats a separate issue
pub fn get_hex_encoded_validator_key(&self) -> String {
hex::encode(self.validator_key.to_bytes())
}

/// Submit all govenerance proposal script inside script_path to the corresponding rest endpoint.
///
/// For all script, we will:
Expand Down Expand Up @@ -161,7 +171,7 @@ impl NetworkConfig {
args.push(framework_path.as_os_str().to_str().unwrap());
};

RunScript::parse_from(args).execute().await?;
RunScript::try_parse_from(args)?.execute().await?;
Ok(())
}

Expand All @@ -174,7 +184,7 @@ impl NetworkConfig {
println!("Creating proposal: {:?}", script_path);

let address_string = format!("{}", self.validator_account);
let privkey_string = hex::encode(self.validator_key.to_bytes());
let privkey_string = self.get_hex_encoded_validator_key();

let metadata_path = TempPath::new();
metadata_path.create_as_file()?;
Expand Down Expand Up @@ -211,11 +221,11 @@ impl NetworkConfig {
if let Some(rev) = &rev_string {
args.push("--framework-git-rev");
args.push(rev.as_str());
SubmitProposal::parse_from(args).execute().await?;
SubmitProposal::try_parse_from(args)?.execute().await?;
} else {
args.push("--framework-local-dir");
args.push(framework_path.as_os_str().to_str().unwrap());
SubmitProposal::parse_from(args).execute().await?;
SubmitProposal::try_parse_from(args)?.execute().await?;
};

// Get proposal id.
Expand All @@ -241,7 +251,7 @@ impl NetworkConfig {
println!("Voting proposal id {:?}", proposal_id);

let address_string = format!("{}", self.validator_account);
let privkey_string = hex::encode(self.validator_key.to_bytes());
let privkey_string = self.get_hex_encoded_validator_key();
let proposal_id = format!("{}", proposal_id);

let args = vec![
Expand All @@ -260,7 +270,7 @@ impl NetworkConfig {
self.endpoint.as_str(),
];

SubmitVote::parse_from(args).execute().await?;
SubmitVote::try_parse_from(args)?.execute().await?;
Ok(())
}

Expand All @@ -286,7 +296,7 @@ impl NetworkConfig {
self.endpoint.as_str(),
];

RunFunction::parse_from(args).execute().await?;
RunFunction::try_parse_from(args)?.execute().await?;
Ok(())
}

Expand All @@ -309,7 +319,7 @@ impl NetworkConfig {
"--url",
self.endpoint.as_str(),
];
RunFunction::parse_from(args).execute().await?;
RunFunction::try_parse_from(args)?.execute().await?;
Ok(())
}

Expand All @@ -320,7 +330,7 @@ impl NetworkConfig {
);

let address_string = format!("{}", self.validator_account);
let privkey_string = hex::encode(self.validator_key.to_bytes());
let privkey_string = self.get_hex_encoded_validator_key();
let proposal_id = format!("{}", proposal_id);

let mut args = vec![
Expand Down Expand Up @@ -351,21 +361,28 @@ impl NetworkConfig {
args.push(framework_path.as_os_str().to_str().unwrap());
};

ExecuteProposal::parse_from(args).execute().await?;
ExecuteProposal::try_parse_from(args)?.execute().await?;
Ok(())
}
}

async fn increase_lockup(validator_address: &str, validator_key: &str) -> Result<()> {
let args = vec![
"--private-key",
validator_key,
"--sender-account",
validator_address,
"--assume-yes",
];
IncreaseLockup::parse_from(args).execute().await?;
Ok(())
async fn increase_lockup(&self) -> Result<()> {
let validator_account = self.validator_account.to_string();
let validator_key = self.get_hex_encoded_validator_key();
let args = vec![
// Ahhhhh this first empty string is very important
// parse_from requires argv[0]
"",
"--sender-account",
validator_account.as_str(),
"--private-key",
validator_key.as_str(),
"--url",
self.endpoint.as_str(),
"--assume-yes",
];
IncreaseLockup::try_parse_from(args)?.execute().await?;
Ok(())
}
}

async fn execute_release(
Expand All @@ -384,12 +401,7 @@ async fn execute_release(
};
release_config.generate_release_proposal_scripts(proposal_folder)?;

// Increase lockup
increase_lockup(
network_config.validator_account.to_string().as_str(),
network_config.validator_key.to_string().as_str(),
)
.await?;
network_config.increase_lockup().await?;

// Execute proposals
for proposal in &release_config.proposals {
Expand Down Expand Up @@ -449,7 +461,7 @@ async fn execute_release(
args.push(framework_path.as_os_str().to_str().unwrap());
};

RunScript::parse_from(args).execute().await?;
RunScript::try_parse_from(args)?.execute().await?;
}
},
};
Expand Down Expand Up @@ -480,3 +492,34 @@ pub async fn validate_config_and_generate_release(
)
.await
}

#[cfg(test)]
pub mod test {
use super::NetworkConfig;
use aptos_crypto::PrivateKey;
use aptos_keygen::KeyGen;
use aptos_types::transaction::authenticator::AuthenticationKey;

#[tokio::test]
pub async fn test_network_config() {
let seed_slice = [0u8; 32];
let mut keygen = KeyGen::from_seed(seed_slice);
let validator_key = keygen.generate_ed25519_private_key();
let validator_account =
AuthenticationKey::ed25519(&validator_key.public_key()).account_address();

let network_info = NetworkConfig {
endpoint: "https://banana.com/".parse().unwrap(),
root_key_path: "".into(),
validator_account,
validator_key,
framework_git_rev: None,
};

let private_key_string = network_info.get_hex_encoded_validator_key();
assert_eq!(
private_key_string.as_str(),
"76b8e0ada0f13d90405d6ae55386bd28bdd219b8a08ded1aa836efcc8b770dc7"
);
}
}

0 comments on commit 3d5db50

Please sign in to comment.