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

[breaking][libra framework] decouple AccountLimits and Balance #5474

Merged
merged 1 commit into from
Aug 8, 2020
Merged

[breaking][libra framework] decouple AccountLimits and Balance #5474

merged 1 commit into from
Aug 8, 2020

Conversation

sblackshear
Copy link
Contributor

@sblackshear sblackshear commented Aug 4, 2020 •
edited
Loading

  • Currently, a ParentVASP needs to have an AccountLimits resource before allowing a child to publish a balance in CoinType. This is confusing and not needed given that AccountLimits are U64_MAX at launch.
  • This PR removes the need to do this. Instead, LibraRoot can send an AdminScript to add AccountLimits to any VASP on which limits need to be imposed.
  • This simplifies the flows for creating new child accounts and adding new currencies (see smaller specs/fewer comments and error codes). I also was able to remove the partial aborts_if for the add_currency script.
  • Also eliminated the tx scripts for updating account limits, since this can now be done via the admin script that installs them.

Breaking changes

  • This is a breaking change in the sense that it changes the bytecode of a few modules and the transaction script allowlist.
  • This is not a breaking change for clients, although the add_currency_to_account script and create_child_vasp script will now succeed in some cases that would previously have failed (e.g., creating a child VASP with add_all_currencies=true from a parent that only has a balance in LBR. This convenience is the primary motivation for the change.

@sblackshear sblackshear added the breaking Any changes that will require restarting the testnet label Aug 4, 2020
n4ss
n4ss approved these changes Aug 4, 2020
View reviewed changes
Copy link
Contributor

@n4ss n4ss left a comment

Choose a reason for hiding this comment

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

That looks great, thanks a lot for that! Please ping me for any changes to the parent/child account creation/management scripts or constraints, it will be easier for us to track changes to our ceremonies and code.

@andll
Copy link

andll commented Aug 4, 2020

Note that for breaking change you need to prefix commit headline as [breaking] now, otherwise it will be rejected by land blocking test

tzakian
tzakian approved these changes Aug 4, 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.

Love red diffs! This looks good to me. cc @moeziniaf for visibility about the removal of the two scripts in case it has AOS implications.

@sblackshear sblackshear changed the title [libra framework] decouple AccountLimits and Balance [breaking][libra framework] decouple AccountLimits and Balance Aug 5, 2020
@MIRAI-bot
Copy link

[move-prover] Move Prover test (language/move-prover/cargo test) on commit 94fcc1e did fail. See details.

@sblackshear sblackshear force-pushed the limit_balance_limits branch 2 times, most recently from 71f5438 to 55e9bef Compare August 6, 2020 19:04
@MIRAI-bot
Copy link

[move-prover] Move Prover test (language/move-prover/cargo test) on commit 55e9bef did fail. See details.

@sblackshear sblackshear force-pushed the limit_balance_limits branch 2 times, most recently from 3f30fe1 to d47cc50 Compare August 6, 2020 20:45
@MIRAI-bot
Copy link

[move-prover] Move Prover test (language/move-prover/cargo test) on commit 3f30fe1 did fail. See details.

@MIRAI-bot
Copy link

[move-prover] Move Prover test (language/move-prover/cargo test) on commit d47cc50 did fail. See details.

sblackshear
sblackshear commented Aug 6, 2020
View reviewed changes
language/stdlib/modules/VASP.move
@@ -154,6 +146,9 @@ module VASP {
global<ChildVASP>(addr).parent_vasp_addr
}
}
define spec_has_account_limits<Token>(addr: address): bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emmazzz: I had to define a spec function for this because the prover wasn't happy with the purity of has_account_limits. Do you know why? Is it because it's not happy with the purity of parent_address?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's right. parent_address has an abort in it, which is currently not supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a conceptual blocker to treating funs with aborts as pure, or just an implementation one? I would guess the latter since @wrwg told me that spec functions can be partial, but wanted to make sure I understand.

@emmazzz
Copy link
Contributor

emmazzz commented Aug 7, 2020

[move-prover] Move Prover test (language/move-prover/cargo test) on commit d47cc50 did fail. See details.

I've seen this failure too after my PR(#5469 ) got merged, and I can't reproduce it locally. Can you reproduce it @sblackshear?

@sblackshear
Copy link
Contributor Author

I did do something

[move-prover] Move Prover test (language/move-prover/cargo test) on commit d47cc50 did fail. See details.

I've seen this failure too after my PR(#5469 ) got merged, and I can't reproduce it locally. Can you reproduce it @sblackshear?

I did accidentally break peer_to_peer_with_metadata in d47cc50, but fixed it in 028d647 (works locally as well).

@moeziniaf
Copy link
Contributor

Love red diffs! This looks good to me. cc @moeziniaf for visibility about the removal of the two scripts in case it has AOS implications.

Thanks - which admin script will be used to add AccountLimits after this PR?

@sblackshear
Copy link
Contributor Author

Love red diffs! This looks good to me. cc @moeziniaf for visibility about the removal of the two scripts in case it has AOS implications.

Thanks - which admin script will be used to add AccountLimits after this PR?

We do not have a script for this yet, but it should be as simple as calling AccountLimits::publish_new_window.

@sblackshear
Copy link
Contributor Author

/land

@sblackshear @bors-libra
[breaking][libra framework] decouple AccountLimits and Balance
a9c78d1 
- Currently, a ParentVASP needs to have an AccountLimits<CoinType> resource before allowing a child to publish a balance in CoinType. This is confusing and not needed given that AccountLimits are U64_MAX at launch.
- This PR removes the need to do this. Instead, LibraRoot can send an AdminScript to add AccountLimits to any VASP on which limits need to be imposed.
- This simplifies the flows for creating new child accounts and adding new currencies (see smaller specs/fewer comments and error codes)
- Also eliminated the tx scripts for updating account limits, since this can now be done via the admin script that installs them.

Closes: diem#5474
@bors-libra bors-libra closed this in a9c78d1 Aug 8, 2020
@bors-libra bors-libra merged commit a9c78d1 into diem:master Aug 8, 2020
@moeziniaf
Copy link
Contributor

Love red diffs! This looks good to me. cc @moeziniaf for visibility about the removal of the two scripts in case it has AOS implications.

Thanks - which admin script will be used to add AccountLimits after this PR?

We do not have a script for this yet, but it should be as simple as calling AccountLimits::publish_new_window.

Is there some limit address which holds default limits? I'm guessing there's a default and after MVP we will want to be able to create/modify new limits (but for now just publish and point an account window to the default limits address?)

@sblackshear sblackshear deleted the limit_balance_limits branch September 21, 2020 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emmazzz emmazzz emmazzz left review comments

@tzakian tzakian tzakian approved these changes

@n4ss n4ss n4ss approved these changes

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.
8 participants
@sblackshear @andll @MIRAI-bot @emmazzz @moeziniaf @tzakian @n4ss @bors-libra