Skip to content

Use network-agnostic Address for encoded contract scripts#94

Merged
greenhat merged 11 commits into
developfrom
issue_88
Aug 16, 2022
Merged

Use network-agnostic Address for encoded contract scripts#94
greenhat merged 11 commits into
developfrom
issue_88

Conversation

@kettlebell

Copy link
Copy Markdown
Collaborator

Close #88

@coveralls

coveralls commented Aug 10, 2022

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 2865742887

  • 75 of 176 (42.61%) changed or added relevant lines in 14 files are covered.
  • 7 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.5%) to 65.692%

Changes Missing Coverage Covered Lines Changed/Added Lines %
core/src/oracle_config.rs 0 1 0.0%
core/src/pool_commands.rs 0 1 0.0%
core/src/main.rs 0 2 0.0%
core/src/cli_commands/extract_reward_tokens.rs 0 3 0.0%
core/src/cli_commands/update_pool.rs 1 4 25.0%
core/src/cli_commands/transfer_oracle_token.rs 0 5 0.0%
core/src/cli_commands/vote_update_pool.rs 1 7 14.29%
core/src/cli_commands/bootstrap.rs 1 9 11.11%
core/src/oracle_state.rs 0 8 0.0%
core/src/cli_commands/prepare_update.rs 3 12 25.0%
Files with Coverage Reduction New Missed Lines %
core/src/box_kind/ballot_box.rs 1 88.39%
core/src/cli_commands/bootstrap.rs 1 86.53%
core/src/cli_commands/update_pool.rs 1 71.3%
core/src/main.rs 4 0%
Totals Coverage Status
Change from base Build 2838643233: 0.5%
Covered Lines: 1955
Relevant Lines: 2976

💛 - Coveralls

@kettlebell

Copy link
Copy Markdown
Collaborator Author

@greenhat this is ready for review if you want to merge this before PR #80.

@greenhat

Copy link
Copy Markdown
Member

I glanced through the code and found that OracleConfig::on_mainnet can be safely removed as well. Also, there are still way too many places using AddressEncoder. Could you please take care of these?

@greenhat

Copy link
Copy Markdown
Member

Also BootstrapConfig::on_mainnet can be safely removed as well.

@kettlebell

Copy link
Copy Markdown
Collaborator Author

Sure will do.

Comment thread core/src/oracle_state.rs Outdated
scans.push(local_scan);
}

let network_prefix =

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It was crucial that OracleConfig::oracle_address is encoded as it allows us to obtain the network prefix used to generate ballot_token_owner_address_str, since we no longer have OracleConfig::on_mainnet.

@kettlebell kettlebell marked this pull request as ready for review August 12, 2022 09:48
@kettlebell kettlebell requested a review from greenhat August 12, 2022 09:48
@kettlebell

kettlebell commented Aug 12, 2022

Copy link
Copy Markdown
Collaborator Author

It's ready for review. One addition I made is to check for consistency of network prefixes in more places, so it will error-out if the operator tries to mix testnet and mainnet addresses.

@greenhat greenhat left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking great! Thank you!
My bad for insisting on Address vs. NetworkAddress. I believe we can ditch more network prefixes. See my comments.

Comment thread core/src/cli_commands/bootstrap.rs Outdated
Comment thread core/src/cli_commands/transfer_oracle_token.rs Outdated
Comment thread core/src/serde.rs Outdated
Comment thread core/src/serde.rs
Comment thread core/src/serde.rs Outdated
Comment thread core/src/serde.rs Outdated
fn from(p: OracleContractParameters) -> Self {
OracleContractParametersSerde {
p2s: p.p2s.to_base58(),
p2s: AddressEncoder::new(NetworkPrefix::Mainnet).address_to_str(&p.p2s),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that eventually, we should switch to base16 serialized ErgoTree for storing contracts in the config files. I mean, the odds of someone setting the new contracts with a testnet prefix are super tiny but non-zero.
Of course, it's out of the scope of this PR.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Made an issue for this: #98

@greenhat

Copy link
Copy Markdown
Member

I also found a hardcoded mainnet in main::get_change_address_from_node(). Could you please take care of this (switch to unchecked parsing)?

@kettlebell

Copy link
Copy Markdown
Collaborator Author

I've addressed all your comments.

@kettlebell kettlebell requested a review from greenhat August 16, 2022 06:35

@greenhat greenhat left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great job! Thank you!

@greenhat greenhat merged commit b6aad70 into develop Aug 16, 2022
@greenhat greenhat deleted the issue_88 branch August 16, 2022 14:52
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.

[300 SigmaUSD] Network agnostic-ness

3 participants