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.

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

[aptos-framework] Add specs for account.move and coin.move #5237

Merged
merged 17 commits into from
Nov 4, 2022
Merged

[aptos-framework] Add specs for account.move and coin.move #5237

merged 17 commits into from
Nov 4, 2022

Conversation

qpb8023
Copy link
Contributor

@qpb8023 qpb8023 commented Oct 24, 2022

Description

This is a part of spec work for Aptos Framework from MoveBit, we updated below files/modules in this PR:

Added specs for module account & coin.

Test Plan

aptos move prove --package-dir ./aptos-framework


This change is Reviewable

Copy link
Contributor

@wrwg wrwg left a comment

Choose a reason for hiding this comment

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

This looks like a good start, but the specifications are incomplete. State updates are not described. If we want to keep state changes unspecified for now, we should add a TODO. However, really the hard work here is to described the state updates ;-)

  • add modifies global<T>(addr) clause for each modified state.
  • specify the post state of the modified slot

This should happen at least to all public functions (eventually).

Copy link
Contributor

@junkil-park junkil-park left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution to the Aptos Framework spec! My few general comments would be:

  • if there are complex specs, please also add some comments (texts) to them for the readability sake
  • please make sure the specs look fine in the generated document. The command to generate the document is:

$ aptos move document --include-impl --include-specs --collapsed-sections

$ cargo test -p framework

@wrwg
Copy link
Contributor

wrwg commented Oct 28, 2022

Thank you for your contribution to the Aptos Framework spec! My few general comments would be:

  • if there are complex specs, please also add some comments (texts) to them for the readability sake
  • please make sure the specs look fine in the generated document. The command to run the document generator is:
$ aptos move document --include-impl --include-specs --collapsed-sections

The documentation generator is run automatically! Don't run it manually please, because it won't have the right options. Documentation is automatically updated if you run cargo test -p framework and placed in <framework>/doc directory.

@rahxephon89 rahxephon89 self-requested a review October 29, 2022 23:14
@qpb8023
Copy link
Contributor Author

qpb8023 commented Oct 31, 2022

Thank you for your contribution to the Aptos Framework spec! My few general comments would be:

  • if there are complex specs, please also add some comments (texts) to them for the readability sake
  • please make sure the specs look fine in the generated document. The command to run the document generator is:
$ aptos move document --include-impl --include-specs --collapsed-sections

The documentation generator is run automatically! Don't run it manually please, because it won't have the right options. Documentation is automatically updated if you run cargo test -p framework and placed in <framework>/doc directory.

Just to ensure: Do I need to add the changes in the doc directory to the commit content?

@junkil-park
Copy link
Contributor

junkil-park commented Oct 31, 2022

Just to ensure: Do I need to add the changes in the doc directory to the commit content?

Yes, the source files and the generated doc need to be synced. So, If you update .move or .spec.move files, please update the doc by cargo test -p framework as well.

Copy link
Contributor

@junkil-park junkil-park left a comment

Choose a reason for hiding this comment

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

Loos good to me. Thank you again for the contribution!

@junkil-park
Copy link
Contributor

It seems that there is a lint test failure in CI. It complains about trailing whitespaces. Could you fix it?
Also, The new Move commit update (#5380) just landed which includes an update on the document generator. Could you rebase and regenerate the document one more time?

@qpb8023
Copy link
Contributor Author

qpb8023 commented Nov 2, 2022

Finished

@movekevin movekevin requested a review from wrwg November 2, 2022 13:38
@junkil-park junkil-park enabled auto-merge (squash) November 4, 2022 03:29
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2022

✅ Forge suite land_blocking success on fa42151098095624c24967eff7c632b259fed9b7

performance benchmark with full nodes : 6853 TPS, 5710 ms latency, 20600 ms p99 latency,(!) expired 4032 out of 2930500 txns
Test Ok

@junkil-park junkil-park merged commit 94574b1 into aptos-labs:main Nov 4, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2022

✅ Forge suite compat success on 2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> fa42151098095624c24967eff7c632b259fed9b7

Compatibility test results for 2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> fa42151098095624c24967eff7c632b259fed9b7 (PR)
1. Check liveness of validators at old version: 2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7603 TPS, 5098 ms latency, 7400 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: fa42151098095624c24967eff7c632b259fed9b7
compatibility::simple-validator-upgrade::single-validator-upgrade : 4554 TPS, 9028 ms latency, 12000 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: fa42151098095624c24967eff7c632b259fed9b7
compatibility::simple-validator-upgrade::half-validator-upgrade : 4566 TPS, 9042 ms latency, 11800 ms p99 latency,no expired txns
4. upgrading second batch to new version: fa42151098095624c24967eff7c632b259fed9b7
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6975 TPS, 5685 ms latency, 9100 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for 2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> fa42151098095624c24967eff7c632b259fed9b7 passed
Test Ok

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.

None yet

6 participants