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

Isolate Python SDK snippets from PR #6356 #6788

Merged
merged 8 commits into from
Mar 19, 2023

Conversation

alnoki
Copy link
Contributor

@alnoki alnoki commented Feb 27, 2023

Per request of @davidiw in #6356 , isolates multisig Python SDK snippets from AMEE content.

The following commands were run in below paths:

aptos-core/:

pre-commit run --all-files

aptos-core/developer-docs-site/:

prettier '**/*.(tsx|ts|js)'
pnpm spellcheck
pnpm lint

aptos-core/ecosystem/python/sdk:

make fmt
make test

Modifications to bcs.py as a result of make fmt were disregarded since the file's contents were not modified per this PR.

Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

Thanks! @clay-aptos want to do a quick run through of the documentation?

# :!:>manifest
[package]
name = 'UpgradeAndGovern'
version = '1.0.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than versioning, could we just label these with more expressive names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the tutorial demonstrates upgrading a package with a multisig, it looks like both need to have the same package.name field in the manifest (otherwise it wouldn't be an upgrade, just publishing two packages with different names that happen to have similar code).

I did experiment with setting package.version to a non-SemVer schema, e.g. version = 'Genesis' and version = 'Upgrade', but the compiler threw:

{
  "Error": "Move compilation failed: Error parsing '[package]' section of manifest: Version is malformed. Versions must be of the form <u64>.<u64>.<u64>, but found 'Genesis'"
}

So I settled on simply placing the packages in the two directories:

  • move-examples/upgrade_and_govern/genesis
  • move-examples/upgrade_and_govern/upgrade

How does this sound?

@@ -0,0 +1,18 @@
// :!:>script
script {
use upgrade_and_govern::parameters;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would avoid placing multiple scripts in the same package and entirely from modules, reason being, they don't keep their names as they get compiled, so you don't know which script is which.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they don't keep their names as they get compiled, so you don't know which script is which

@davidiw I'm a bit confused by this guidance, as the upgrade package has two scripts, set_and_transfer.move and set_only.move, which respectively compile into set_and_transfer.mv and set_only.mv as expected.

I suspect the hypothetical name loss you refer to is a result of using main as the main function name for every script in a Move package:

script {
    fun main( // <- main function name
    //  ^ this PR uses either `set_only` or `set_and_transfer`, but
    //    name loss would result if both had `main`
        ...
    ) {
        ...
    }
}

This is analogous to using the same module definition block across multiple module .move files

module named_address::a_repeated_module_name {
    //                ^ this compiles into `a_repeated_module_name.mv`
    //                  regardless of the `.move` filename
    ...
}

What are your thoughts?

@@ -79,6 +80,37 @@ def public_key(self) -> ed25519.PublicKey:
return self.private_key.public_key()


class RotationProofChallenge:
Copy link
Contributor

Choose a reason for hiding this comment

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

so I was suggesting that we emit out a chunk of bytes from say

let rotation_msg = bcs::to_bytes(&rotation_proof).unwrap();

and dumping this to hex and importing that hex string here... that way we can be confident that it works as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidiw how does this look?

https://github.com/alnoki/aptos-core/blob/dc8ac59fe89ecf04f2b6187a7e2ae83179cde23a/ecosystem/python/sdk/aptos_sdk/account.py#L131-L160

The classes have been verified e2e on devnet using the example script broken down in the tutorial, so I just emitted some bytes and made a test for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

great

ecosystem/python/sdk/aptos_sdk/ed25519.py Show resolved Hide resolved
ecosystem/python/sdk/aptos_sdk/ed25519.py Show resolved Hide resolved
ecosystem/python/sdk/aptos_sdk/ed25519.py Show resolved Hide resolved
ecosystem/python/sdk/aptos_sdk/ed25519.py Show resolved Hide resolved
ecosystem/python/sdk/examples/multisig.py Outdated Show resolved Hide resolved
ecosystem/python/sdk/examples/multisig.py Outdated Show resolved Hide resolved
ecosystem/python/sdk/examples/multisig.py Outdated Show resolved Hide resolved
Copy link
Contributor

@clay-aptos clay-aptos left a comment

Choose a reason for hiding this comment

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

Thanks so much, Al. The docs LGTM with some tiny changes I made in direct commits and a fix to the examples directory I flag in a comment here. Thanks again! I look forward to adding this to our Community Highlights list!

developer-docs-site/docs/tutorials/first-multisig.md Outdated Show resolved Hide resolved
@clay-aptos
Copy link
Contributor

Thanks! @clay-aptos want to do a quick run through of the documentation?

Definitely! All done. Thanks again, @alnoki !

@clay-aptos
Copy link
Contributor

Hi @alnoki , I am noting we launched this:
https://aptos.dev/community/contributions/community-highlights

And would love to have this online. Please let me know how O can help. Thanks!

@alnoki
Copy link
Contributor Author

alnoki commented Mar 8, 2023

Hi @alnoki , I am noting we launched this: https://aptos.dev/community/contributions/community-highlights

And would love to have this online. Please let me know how O can help. Thanks!

@clay-aptos thank you for your patience during all of the delays during Aptos Week at Mountain DAO and then ETH Denver. I've about 80% done with a commit that addresses comments from you and @davidiw above and am hoping to push in the next few days or so.

@alnoki
Copy link
Contributor Author

alnoki commented Mar 16, 2023

@davidiw @clay-aptos I believe I've addressed your review comments above: is there anything else you need to move this along?

@clay-aptos
Copy link
Contributor

@davidiw @clay-aptos I believe I've addressed your review comments above: is there anything else you need to move this along?

Hi @alnoki , this LGTM! Thank you. @davidiw ?

Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

thanks!!

@@ -79,6 +80,37 @@ def public_key(self) -> ed25519.PublicKey:
return self.private_key.public_key()


class RotationProofChallenge:
Copy link
Contributor

Choose a reason for hiding this comment

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

great

print(f"Bob: {bob.public_key()}")
print(f"Chad: {chad.public_key()}") # <:!:section_1

wait()
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the point of these waits? we want these to run in a test environment... we need to quickly follow up this PR with something that validates this on every PR -- these quickly bitrot or start failing due to other ninjas

@davidiw davidiw force-pushed the multisig-snippets-squashed branch from 124bf3c to 3bbd3f3 Compare March 19, 2023 16:40
@davidiw davidiw enabled auto-merge (rebase) March 19, 2023 16:40
@davidiw
Copy link
Contributor

davidiw commented Mar 19, 2023

realized the tutorial is interactive... this will bitrot, we need to consider some other means, even if that is that we keep pre-compiled versions around.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

✅ Forge suite land_blocking success on 3bbd3f3c4aa6a1d7eca5137e1c45f3f72c45d2e5

performance benchmark with full nodes : 5664 TPS, 7016 ms latency, 12000 ms p99 latency,no expired txns
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> 3bbd3f3c4aa6a1d7eca5137e1c45f3f72c45d2e5

Compatibility test results for cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> 3bbd3f3c4aa6a1d7eca5137e1c45f3f72c45d2e5 (PR)
Upgrade the nodes to version: 3bbd3f3c4aa6a1d7eca5137e1c45f3f72c45d2e5
framework_upgrade::framework-upgrade::full-framework-upgrade : 6671 TPS, 5850 ms latency, 8600 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> 3bbd3f3c4aa6a1d7eca5137e1c45f3f72c45d2e5 passed
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 3bbd3f3c4aa6a1d7eca5137e1c45f3f72c45d2e5

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 3bbd3f3c4aa6a1d7eca5137e1c45f3f72c45d2e5 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7916 TPS, 4852 ms latency, 6500 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 3bbd3f3c4aa6a1d7eca5137e1c45f3f72c45d2e5
compatibility::simple-validator-upgrade::single-validator-upgrade : 4924 TPS, 8117 ms latency, 11800 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 3bbd3f3c4aa6a1d7eca5137e1c45f3f72c45d2e5
compatibility::simple-validator-upgrade::half-validator-upgrade : 4531 TPS, 9277 ms latency, 12200 ms p99 latency,no expired txns
4. upgrading second batch to new version: 3bbd3f3c4aa6a1d7eca5137e1c45f3f72c45d2e5
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6572 TPS, 5807 ms latency, 10500 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 3bbd3f3c4aa6a1d7eca5137e1c45f3f72c45d2e5 passed
Test Ok

@clay-aptos
Copy link
Contributor

Just noting we are announcing and linking in:
#7308 (comment)

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.

3 participants