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

[docs] Improved aggregator Move documentation #5659

Merged
merged 5 commits into from
Dec 4, 2022
Merged

[docs] Improved aggregator Move documentation #5659

merged 5 commits into from
Dec 4, 2022

Conversation

georgemitenkov
Copy link
Contributor

Description

Moved design rationale for aggregators into a separate readme, and fixed the Move documentation accordingly.

Test Plan

georgemitenkov
georgemitenkov commented Nov 21, 2022
View reviewed changes
aptos-move/framework/aptos-framework/sources/aggregator/aggregator.move
const EAGGREGATOR_UNDERFLOW: u64 = 2;

/// When aggregator feature is not supported (raised by native code).
/// When aggregator feature is not supported. Raised by native code.
const ENOT_SUPPORTED: u64 = 3;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that this is not used anywhere, also to in Move all constants are private so dropping this should be compatible?

Copy link
Contributor

Choose a reason for hiding this comment

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

All error codes' comments are auto-updated in the error mapping (done as part of upgrading existing modules). These are shown to end users as error messages.

Copy link
Contributor Author

@georgemitenkov georgemitenkov Nov 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I mean would it be safe to drop const ENOT_SUPPORTED: u64 = 3; all together? If the comments are updated in the mapping, not sure we can do that...

movekevin
movekevin approved these changes Nov 29, 2022
View reviewed changes
aptos-move/framework/aptos-framework/sources/aggregator/README.md
@@ -0,0 +1,47 @@
# Design Rationale
Copy link
Contributor

Choose a reason for hiding this comment

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

May be this can be moved into dev docs? cc @clay-aptos

Copy link
Contributor

Choose a reason for hiding this comment

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

May be this can be moved into dev docs? cc @clay-aptos

Yes, all of these docs should be considered for Aptos.dev. See my issue for this in:
#5577

Just ping that issue when you are ready to work on it. We have myriad docs in the repo that are not linked from Aptos.dev, it seems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! Maybe this can wait until the blogpost first then.

aptos-move/framework/aptos-framework/sources/aggregator/aggregator.move Outdated Show resolved Hide resolved
aptos-move/framework/aptos-framework/sources/aggregator/aggregator.move
const EAGGREGATOR_UNDERFLOW: u64 = 2;

/// When aggregator feature is not supported (raised by native code).
/// When aggregator feature is not supported. Raised by native code.
const ENOT_SUPPORTED: u64 = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

All error codes' comments are auto-updated in the error mapping (done as part of upgrading existing modules). These are shown to end users as error messages.

aptos-move/framework/aptos-framework/sources/aggregator/aggregator.move Outdated Show resolved Hide resolved
aptos-move/framework/aptos-framework/sources/aggregator/aggregator.move Outdated Show resolved Hide resolved
aptos-move/framework/aptos-framework/sources/aggregator/aggregator_factory.move Outdated Show resolved Hide resolved
aptos-move/framework/aptos-framework/sources/aggregator/optional_aggregator.move Outdated
struct Integer has store {
value: u128,
limit: u128,
}

/// Creates a new integer
Copy link
Contributor

Choose a reason for hiding this comment

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

This is rather a lazy comment. Can you explain a bit more on what this struct is for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this comment is for the new function and not for the struct (struct has a more explicit one). I guess I would just remove it then or state that a new integer is created that follows same overflow semantics as aggregator.

@clay-aptos clay-aptos mentioned this pull request Nov 29, 2022
Closed
@davidiw davidiw enabled auto-merge (squash) December 3, 2022 23:26
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2022

✅ Forge suite land_blocking success on f8c0f97ab421075bee9e65ee418f7011dd7fa2cc

performance benchmark with full nodes : 6612 TPS, 5993 ms latency, 9900 ms p99 latency,(!) expired 120 out of 2823460 txns
Test Ok

@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2022

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> f8c0f97ab421075bee9e65ee418f7011dd7fa2cc

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> f8c0f97ab421075bee9e65ee418f7011dd7fa2cc (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7225 TPS, 5325 ms latency, 8200 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: f8c0f97ab421075bee9e65ee418f7011dd7fa2cc
compatibility::simple-validator-upgrade::single-validator-upgrade : 4349 TPS, 8946 ms latency, 12500 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: f8c0f97ab421075bee9e65ee418f7011dd7fa2cc
compatibility::simple-validator-upgrade::half-validator-upgrade : 4466 TPS, 9110 ms latency, 12700 ms p99 latency,no expired txns
4. upgrading second batch to new version: f8c0f97ab421075bee9e65ee418f7011dd7fa2cc
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6435 TPS, 6065 ms latency, 10000 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> f8c0f97ab421075bee9e65ee418f7011dd7fa2cc passed
Test Ok

@davidiw davidiw merged commit 2487961 into main Dec 4, 2022
@davidiw davidiw deleted the george/aggregator-docs branch December 4, 2022 00:58
@Markuze Markuze mentioned this pull request Dec 5, 2022
Closed
areshand pushed a commit to areshand/aptos-core-1 that referenced this pull request Dec 18, 2022
@georgemitenkov @areshand
[docs] Improved aggregator Move documentation (aptos-labs#5659)
fb54c0e 
* [docs] Improved aggregator documentation

* [docs] Typo fixes in aggregator documentation

* [docs] addressed comments
@Markuze Markuze mentioned this pull request Dec 26, 2022
Closed
@Markuze Markuze mentioned this pull request Jan 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clay-aptos clay-aptos clay-aptos left review comments

@movekevin movekevin movekevin approved these changes

@zekun000 zekun000 zekun000 approved these changes

@gelash gelash Awaiting requested review from gelash

@davidiw davidiw Awaiting requested review from davidiw davidiw is a code owner

@wrwg wrwg Awaiting requested review from wrwg wrwg is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@georgemitenkov @zekun000 @movekevin @clay-aptos @davidiw