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

feat: Add deposit yocto #822

Merged
merged 2 commits into from Jul 27, 2021
Merged

feat: Add deposit yocto #822

merged 2 commits into from Jul 27, 2021

Conversation

ChaoticTempest
Copy link
Member

@ChaoticTempest ChaoticTempest commented Jul 20, 2021

Closes #751

This only adds the depositYocto option to the call command.

I didn't add it to other commands since they were still using amount as an option. Should we go ahead and change those to deposit/depositYocto as well?

Usage:

$ near call $id $fn_name $params --accountId $id --depositYocto 10000000000000000000000000

Versus the equivalent with deposit:

$ near call $id $fn_name $params --accountId $id --deposit 10

@ChaoticTempest
Copy link
Member Author

Also, Travis CI is failing on my usage of null coalescing. What JS version are we using?

@@ -19,6 +19,11 @@ module.exports = {
default: '0',
alias: 'amount'
})
.option('depositYocto', {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that we should add a separate option here.
And we definitely should not use it as a flag.
@mikedotexe I do not understand the motivation in your comment here: #751 (comment)

I would say, that we need to design it the next way:
near call ... ... --deposit 000000001 it would be in N, for backward compatibility + deprecation warning.
near call ... ... deposit 1N
near call ... ... deposit 1yN for deposit in yoctoNEAR

If we know for sure that deposit is always in yoctoNEAR, then let's do it the next way:
near call ... ... --depositYocto 1 + add deprecation warning for --deposit
This one will be less flexible.

+, if we will have --deposit and --depositYocto at the same time we should not allow to specify them both (there is a conflicting functionality in yargs).

@ChaoticTempest please, provide an example of this option usage in the description.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the PR description or somewhere else?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, let's put it there :)

@volovyks
Copy link
Collaborator

Also, Travis CI is failing on my usage of null coalescing. What JS version are we using?

You should be able to check it somewhere in the CI setup. Do you have an access?

@volovyks
Copy link
Collaborator

I didn't add it to other commands since they were still using amount as an option. Should we go ahead and change those to deposit/depositYocto as well?

I believe that amount options do not always mean deposit. But if you thing that it should be renamed somewhere - that would be cool 👍 :)

@ChaoticTempest
Copy link
Member Author

You should be able to check it somewhere in the CI setup. Do you have an access?

Oh I see now, it's using node js 12

I believe that amount options do not always mean deposit. But if you thing that it should be renamed somewhere - that would be cool 👍 :)

Ahh, I must've been mistaken then when I looked at it. No need to change it to deposit then

@mikedotexe mikedotexe requested a review from volovyks July 27, 2021 06:54
@volovyks volovyks merged commit f5e70fc into master Jul 27, 2021
@mikedotexe mikedotexe deleted the feature/deposit-yocto branch July 27, 2021 16:25
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.

Allow to specify amounts in yN
3 participants