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

Add doc explaining how to use Move scripts #7449

Merged
merged 5 commits into from
Mar 30, 2023
Merged

Conversation

banool
Copy link
Contributor

@banool banool commented Mar 28, 2023

Description

This is mostly copied from my Stack Overflow post here (which I will update to point to the docs once this lands). I made a few tweaks but it is mostly unchanged. I like the question -> answer format, so I'd prefer to keep it that way.

Test Plan

CI.

@banool banool force-pushed the banool/move-scripts-doc branch 3 times, most recently from 7e39786 to d8cc15d Compare March 28, 2023 16:42
@banool banool marked this pull request as ready for review March 28, 2023 16:43
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, Daniel! I made some slight edits and added a short intro. Other than a comment seeking clarity, this LGTM!

Also, I will note this addition in the related and add you for potential next steps:
#6318


That entry should go in a Move module file.

Now you need to determine:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it useful to list these questions here instead of directly below along with the answers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this originally came from Stack Overflow and is therefore meant to be in a Q&A format. Already it's a bit more confusing because it is no longer "I have this problem / question" and "here is the answer", but "you might have this problem" and "here is the answer".

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay

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'm open to changes, idk what's best now, the style of this doc has changed a lot now lol

Copy link
Contributor

Choose a reason for hiding this comment

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

Up to you. I don't feel strongly about this. Was just curious

Copy link
Contributor

Choose a reason for hiding this comment

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

I moved the questions to be the intro of the Execution section (which was my gut response yet trying not to trample Daniel's fine work).

cd testing
```

2. Set up the Aptos CLI:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why now link to the CLI doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm? Not sure what you mean.

Copy link
Contributor

Choose a reason for hiding this comment

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

We're just duplicating the steps for setting up the CLI so a ref to the setup CLI page might be more maintainable

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 think doing that for just a single step would be confusing, we don't have a doc that tells you only how to use aptos init. A quick search in the docs reveals that there are like 20 places where we suggest people use aptos init directly in the guide.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see

Copy link
Contributor

Choose a reason for hiding this comment

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

I felt the same way and so wrote:
https://aptos.dev/guides/get-test-funds#create-an-aptos-account

To be fair, this was existent but sparse and mixed into a rather gigantic doc (that I am hoping @gregnazario will let me break into sub-pages someday):
https://aptos.dev/cli-tools/aptos-cli-tool/use-aptos-cli#step-1-run-aptos-init

I can also link to one of the above yet agree with Daniel he's supplying the minimum details to keep the user going. This is why you find aptos init everywhere, I imagine. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a nice doc, feel free to add a link to that if it makes sense :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Done!

@banool banool force-pushed the banool/move-scripts-doc branch from 662b265 to 4a7e06e Compare March 29, 2023 15:43

let balance = coin::balance<aptos_coin::AptosCoin>(src_addr);
if (balance < desired_balance) {
aptos_account::transfer(src, dest, desired_balance - balance);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Might as well demonstrate two calls, especially those against public fun functions that are not callable as normal transactions. For example:

let withdraw_coins = aptos_account::withdraw_coins(...);
aptos_account::deposit_coins(dest, withdraw_coins);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya sure why not

@banool banool enabled auto-merge (squash) March 29, 2023 17:15
@clay-aptos
Copy link
Contributor

Related:
#7470

@banool banool disabled auto-merge March 30, 2023 09:50
@banool banool merged commit b15019e into main Mar 30, 2023
@banool banool deleted the banool/move-scripts-doc branch March 30, 2023 09:50
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.

4 participants