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

[testnet] switch testnet minting from AssocRoot account to TreasuryCompliance account #4458

Closed
wants to merge 1 commit into from
Closed

[testnet] switch testnet minting from AssocRoot account to TreasuryCompliance account #4458

wants to merge 1 commit into from

Conversation

sblackshear
Copy link
Contributor

@sblackshear sblackshear commented Jun 12, 2020 •
edited

Today, minting in testnet and in all tests is performed from the AssocRoot account 0xA550C18. This is different from the production configuration, where there is a single MintCapability given to the TreasuryCompliance account 0xB1E55ED.

This PR changes tests/testnet to reflect production.

  • remove Libra::grant_mint_capability_to_association. This was a temporary hack that violates an important invariant: the MintCapability for a given currency should be globally unique. This property should now be true.
  • Add treasury_compliance_account to the client CLI. Minting operations now use this account. It uses the same keypair as AssocRoot for now (but need not)
  • Fix all the tests that used 0xA550C18 for minting

tzakian
tzakian approved these changes Jun 12, 2020
View reviewed changes
Copy link
Contributor

@tzakian tzakian left a comment

Choose a reason for hiding this comment

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

Looks good to me. Only thing I'm wondering is if this breaks the faucet service for the client?

language/stdlib/modules/Libra.move
Comment on lines -182 to -187

// TODO: temporary, we should ideally make MintCapability unique eventually...
public fun grant_mint_capability_to_association<CoinType>(association: &signer) {
assert_assoc_and_currency<CoinType>(association);
move_to(association, MintCapability<CoinType>{})
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@sblackshear
Copy link
Contributor Author

That is a very good point. I will check the faucet code and make sure--ideally it should be as simple as sending faucet txes from 0xB1E55ED instead of 0xA550C18.

@bors-libra
Copy link
Contributor

☔ The latest upstream changes (presumably 50ee2bd) made this pull request unmergeable. Please resolve the merge conflicts.

@cargo-dep-bot
Copy link

cargo-dep-bot bot commented Jun 12, 2020

This PR made the following dependency changes:

Added Packages (Duplicate versions in '()'):
	enum_dispatch 0.3.1

@sblackshear sblackshear force-pushed the unique_mint_cap branch 2 times, most recently from 92e6866 to b16e321 Compare June 12, 2020 17:38
@sblackshear sblackshear changed the title [move core modules] remove Libra::grant_mint_capability_to_association [testnet] switch testnet minting from AssocRoot account to TreasuryCompliance account Jun 12, 2020
@sblackshear
Copy link
Contributor Author

CC @christinacelee @thefallentree to make sure this won't break the faucet service.

@sblackshear sblackshear force-pushed the unique_mint_cap branch 2 times, most recently from 3e9b6db to 5416d19 Compare June 12, 2020 18:43
@sblackshear sblackshear added the breaking Any changes that will require restarting the testnet label Jun 12, 2020
@thefallentree
Copy link
Contributor

looks good to me since you already modified CLI which faucet uses.

@sblackshear
Copy link
Contributor Author

@bors-libra r=tzakian

@bors-libra
Copy link
Contributor

📌 Commit 97afddb has been approved by tzakian

@bors-libra
Copy link
Contributor

⌛ Testing commit 97afddb with merge 5004a6a...

bors-libra pushed a commit that referenced this pull request Jun 12, 2020
@sblackshear @bors-libra
[move core modules] remove Libra::grant_mint_capability_to_association
5004a6a 
This was a temporary hack that violates an important invariant: the MintCapability for a given currency should be globally unique. This property should now be true.

Closes: #4458
Approved by: tzakian
@bors-libra
Copy link
Contributor

💔 Test failed - checks-circle_commit_workflow

@sblackshear sblackshear force-pushed the unique_mint_cap branch 3 times, most recently from 2f3011b to 3b7120f Compare June 12, 2020 23:13
@sblackshear
Copy link
Contributor Author

@bors-libra r=tzakian

@bors-libra
Copy link
Contributor

📌 Commit 3b7120f has been approved by tzakian

@bors-libra
Copy link
Contributor

⌛ Testing commit 54d709c with merge e6f3892...

bors-libra pushed a commit that referenced this pull request Jun 12, 2020
@sblackshear @bors-libra
[move core modules] remove Libra::grant_mint_capability_to_association
e6f3892 
This was a temporary hack that violates an important invariant: the MintCapability for a given currency should be globally unique. This property should now be true.

Closes: #4458
Approved by: tzakian
@github-actions
Copy link

Cluster Test failed - test report processing failed. See https://github.com/libra/libra/actions/runs/133853596
Repro cmd:

./scripts/cti --tag land_e6f3892a --run bench

@bors-libra
Copy link
Contributor

💔 Test failed - checks-actions_land_blocking_test

@bors-libra
Copy link
Contributor

☔ The latest upstream changes (presumably 44dcf38) made this pull request unmergeable. Please resolve the merge conflicts.

@sblackshear sblackshear force-pushed the unique_mint_cap branch 3 times, most recently from 8faa84f to a7973ce Compare June 13, 2020 16:38
@sblackshear
Copy link
Contributor Author

@bors-libra r=tzakian

@bors-libra
Copy link
Contributor

📌 Commit a7973ce has been approved by tzakian

@bors-libra
Copy link
Contributor

⌛ Testing commit a7973ce with merge 9c47cb7...

bors-libra pushed a commit that referenced this pull request Jun 13, 2020
@sblackshear @bors-libra
[move core modules] remove Libra::grant_mint_capability_to_association
9c47cb7 
This was a temporary hack that violates an important invariant: the MintCapability for a given currency should be globally unique. This property should now be true.

Closes: #4458
Approved by: tzakian
@github-actions
Copy link

Cluster Test failed - test report processing failed. See https://github.com/libra/libra/actions/runs/134391884
Repro cmd:

./scripts/cti --tag land_9c47cb70 --run bench

@bors-libra
Copy link
Contributor

💔 Test failed - checks-actions_land_blocking_test

@github-actions
Copy link

Cluster Test failed - test report processing failed. See https://github.com/libra/libra/actions/runs/134391884
Repro cmd:

./scripts/cti --tag land_9c47cb70 --run bench

@sblackshear
Copy link
Contributor Author

@bors-libra r=tzakian

@bors-libra
Copy link
Contributor

📌 Commit f1f0ab9 has been approved by tzakian

@bors-libra
Copy link
Contributor

⌛ Testing commit f1f0ab9 with merge 8fe5535...

bors-libra pushed a commit that referenced this pull request Jun 13, 2020
@sblackshear @bors-libra
[move core modules] remove Libra::grant_mint_capability_to_association
8fe5535 
This was a temporary hack that violates an important invariant: the MintCapability for a given currency should be globally unique. This property should now be true.

Closes: #4458
Approved by: tzakian
@sblackshear
[move core modules] remove Libra::grant_mint_capability_to_association
073c1d5 
This was a temporary hack that violates an important invariant: the MintCapability for a given currency should be globally unique. This property should now be true.
@sblackshear
Copy link
Contributor Author

@bors-libra r=tzakian

@bors-libra
Copy link
Contributor

📌 Commit 073c1d5 has been approved by tzakian

@bors-libra
Copy link
Contributor

⌛ Testing commit 073c1d5 with merge 9436dd4...

@github-actions
Copy link

Cluster Test Result

all up : 1342 TPS, 3358 ms latency, 3950 ms p99 latency, no expired txns

Repro cmd:

./scripts/cti --tag land_8fe55355 --run bench

@github-actions
Copy link

Cluster Test Result

all up : 1348 TPS, 3351 ms latency, 4400 ms p99 latency, no expired txns

Repro cmd:

./scripts/cti --tag land_9436dd45 --run bench

@bors-libra
Copy link
Contributor

☀️ Test successful - checks-actions_land_blocking_test, checks-circle_commit_workflow
Approved by: tzakian
Pushing 9436dd4 to master...

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

@tzakian tzakian tzakian approved these changes

@DavidLDill DavidLDill Awaiting requested review from DavidLDill

@emmazzz emmazzz Awaiting requested review from emmazzz

@christinacelee christinacelee Awaiting requested review from christinacelee

@thefallentree thefallentree Awaiting requested review from thefallentree

Assignees
No one assigned
Labels
breaking Any changes that will require restarting the testnet cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sblackshear @bors-libra @thefallentree @tzakian