Addresses subdomains in create_account, dev-deploy #324

mikedotexe · 2020-04-23T01:01:16Z

Addresses #307.
Note that there is still an underlying issue when the test tries to deploy a contract:

Error:  LackBalanceForState [Error]: The account dev-1587603497974.test.near wouldn't have enough balance to cover storage, required to have 16543700000000000000000
    at Object.parseRpcError (/Users/mike/near/near-shell/node_modules/near-api-js/lib/utils/rpc_errors.js:25:19)
    at Account.signAndSendTransaction (/Users/mike/near/near-shell/node_modules/near-api-js/lib/account.js:114:36)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async LocalAccountCreator.createAccount (/Users/mike/near/near-shell/node_modules/near-api-js/lib/account_creator.js:23:9)
    at async createDevAccountIfNeeded (/Users/mike/near/near-shell/commands/dev-deploy.js:67:5)
    at async devDeploy (/Users/mike/near/near-shell/commands/dev-deploy.js:34:23)
    at async Object.handler (/Users/mike/near/near-shell/utils/exit-on-error.js:4:9) {
  type: 'LackBalanceForState',
  index: null,
  amount: '16543700000000000000000',
  signer_id: undefined,
  account_id: 'dev-1587603497974.test.near'

Edit: resolved, see Vlad's comment below.

Edit2:
This ended up being a bit of a compromise, knowing that long-term, we'll be allowing TLA's that are based on other chains' account addresses. Yet at the same time, we want to make clear to the end user that most of the time they'll want to approach account creation as subaccounts.

Here's a list of commands run and the results to demonstrate:
./bin/near create_account hihihi --masterAccount asdf.test
results in:

Top-level accounts must be greater than 32 characters.
Note: this is for advanced usage only. Typical account names are of the form:
app.alice.test, where the masterAccount shares the top-level account (.test).

./bin/near create_account hihihiveryveryverylongnamethisisunusual --masterAccount asdf.test
passes

NODE_ENV=betanet ./bin/near create_account mike.asdf.test --masterAccount asdf.test
passes but throws the message:

NOTE: In most cases, when connected to "betanet", account and masterAccount will end in ".beta"

./bin/near create_account mike.asdf.test --masterAccount asdf.test
succeeds

./bin/near create_account hi.mike.asdf.test --masterAccount mike.asdf.test
succeeds

./bin/near create_account hi.mike.asdf.test --masterAccount mike.asdf.purvis
results in:

New account doesn't share the same top-level account. Expecting account name to end in ".mike.asdf.purvis"

./bin/near create_account hi.mike.asdf.purvis --masterAccount mike.asdf.test
results in:

New account doesn't share the same top-level account. Expecting account name to end in ".mike.asdf.test"

…nd throws if needed

# Conflicts: # commands/create-account.js

vgrichina · 2020-04-23T01:10:00Z

@mikedotexe

Note that there is still an underlying issue when the test tries to deploy a contract:

I think it should be fixed by this 7e31bfb, try rebasing/merging with master

mikedotexe · 2020-04-23T16:56:48Z

Update: I think I am not understanding what a TLA is according to @bowenwang1996 who had a comment here:
#307 (comment)

well, hihihihihihi.test is unfortunately not a TLA

behaviary · 2020-04-23T17:03:12Z

commands/create-account.js

@@ -32,6 +38,26 @@ module.exports = {

 async function createAccount(options) {


You should add a check that the suffix matches the network that the user is talking to.

other than that, looks good.

vgrichina · 2020-04-23T20:27:04Z

commands/create-account.js

 const exitOnError = require('../utils/exit-on-error');
 const connect = require('../utils/connect');
 const { KeyPair } = require('near-api-js');
+const NEAR_ENV_SUFFIXES = [
+    'near',


I think this should go to config. Maybe even just reuse masterAccount for this, but use helperUrl for account creation when available.

helperUrl? I thought that is only used in the dev-deploy. As far as I know, the create_account command will always need a masterAccount right?

it is generally used when you create accounts using near.createAccount https://github.com/near/near-api-js/blob/f1dcf73ff3549fe7bc7caf35c7c6e43fbc4a2703/src/near.ts#L59
https://github.com/near/near-api-js/blob/f1dcf73ff3549fe7bc7caf35c7c6e43fbc4a2703/src/near.ts#L37

Though really the only hard statement here is that name suffix should be configurable, I'm not sure it should be universally be the same as masterAccount.

Gotcha, interesting. Currently if you try passing just the helperUrl yargs is set to fail because masterAccount is required.

./bin/near create_account mike.asdf.test --accountHelper https://helper.testnet.nearprotocol.com
gives

Missing required argument: masterAccount

I feel like that'd be a good enhancement for another issue. Is it cool if we do this in another PR?

I do like the idea of using config for those, by the way, and I'll be pushing up a change that checks config's networkId against it.

and I'll be pushing up a change that checks config's networkId against it.

not sure what do you mean

I feel like that'd be a good enhancement for another issue. Is it cool if we do this in another PR?

sure, my main point here is just so that we avoid hardcoding suffixes in shell. If they are in config – they can easily be altered for a given project, etc. Probably should also be passable as argument (again so that it's easy to override when needed).

I agree, I just pushed up a commit that essentially suggests to the user that most of the time if they're on betanet, the account will end in .beta. But it's not blocking. We can't make it a hard requirement because of the custom TLAs, too. Good point

vgrichina · 2020-04-23T20:28:30Z

commands/create-account.js

+            console.log(`Top-level accounts (not ending in .near, .test, etc) must be greater than ${TLA_MIN_LENGTH} characters`);
+            return;
+        }
+    } else if (splitAccount.length === 3) {


length isn't limited to 3, can be easily foo.bar.baz.alice.near if you have bar.baz.alice.near as masterAccount

thanks, this is helpful feedback

vgrichina · 2020-04-23T20:30:02Z

commands/dev-deploy.js

@@ -67,7 +67,7 @@ async function createDevAccountIfNeeded({ near, keyStore, networkId, init }) {
        }
    }

-    const accountId = `dev-${Date.now()}`;
+    const accountId = `dev-${Date.now()}.test.near`;


I think we should take masterAccount from config, suffix here not necessarily test.near.

…a's >= 32

mikedotexe · 2020-04-24T02:31:09Z

@vgrichina when you get a sec, this is ready for re-review. I just moved it out of Draft, so want to ping as I don't know if Github alerts reviewers upon that action.

# Conflicts: # commands/create-account.js

mikedotexe added 4 commits April 22, 2020 17:37

dev-deploy to use x.test.near

7c75f74

create-account tells user of validation requirements, gives message a…

c87cdb8

…nd throws if needed

tests create accounts as subdomains of test.near

95a4fea

Merge branch 'master' into hotfix/307-subdomains

92cc593

# Conflicts: # commands/create-account.js

mikedotexe added 2 commits April 22, 2020 19:52

lint fixes

112676c

remove create_account error, simply inform and return

2c82feb

mikedotexe requested a review from vgrichina April 23, 2020 14:52

mikedotexe changed the title ~~WIP - Addresses subdomains in create_account, dev-deploy~~ Addresses subdomains in create_account, dev-deploy Apr 23, 2020

mikedotexe requested a review from behaviary April 23, 2020 16:35

mikedotexe marked this pull request as draft April 23, 2020 16:57

behaviary reviewed Apr 23, 2020

View reviewed changes

behaviary approved these changes Apr 23, 2020

View reviewed changes

vgrichina reviewed Apr 23, 2020

View reviewed changes

address growing account names, warn against network inconsistency, tl…

8d39834

…a's >= 32

mikedotexe marked this pull request as ready for review April 24, 2020 02:30

Merge branch 'master' into hotfix/307-subdomains

9e217cc

# Conflicts: # commands/create-account.js

mikedotexe merged commit 122a5fb into master Apr 26, 2020

mikedotexe deleted the hotfix/307-subdomains branch April 26, 2020 21:51

This was referenced Apr 27, 2020

Revert "Addresses subdomains in create_account, dev-deploy" #330

Merged

Hotfix/307 subdomains #331

Closed

use subaccounts and provide suggestions, fix tests #332

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addresses subdomains in create_account, dev-deploy #324

Addresses subdomains in create_account, dev-deploy #324

mikedotexe commented Apr 23, 2020 •

edited by gitpod-io bot

Loading

vgrichina commented Apr 23, 2020

mikedotexe commented Apr 23, 2020 •

edited

Loading

behaviary Apr 23, 2020

behaviary Apr 23, 2020

vgrichina Apr 23, 2020

mikedotexe Apr 23, 2020

vgrichina Apr 23, 2020

mikedotexe Apr 24, 2020 •

edited

Loading

mikedotexe Apr 24, 2020

vgrichina Apr 24, 2020

vgrichina Apr 24, 2020

mikedotexe Apr 24, 2020

vgrichina Apr 23, 2020

mikedotexe Apr 24, 2020

vgrichina Apr 23, 2020

mikedotexe commented Apr 24, 2020

		@@ -32,6 +38,26 @@ module.exports = {

		async function createAccount(options) {

Addresses subdomains in create_account, dev-deploy #324

Addresses subdomains in create_account, dev-deploy #324

Conversation

mikedotexe commented Apr 23, 2020 • edited by gitpod-io bot Loading

vgrichina commented Apr 23, 2020

mikedotexe commented Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedotexe Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedotexe commented Apr 24, 2020

mikedotexe commented Apr 23, 2020 •

edited by gitpod-io bot

Loading

mikedotexe commented Apr 23, 2020 •

edited

Loading

mikedotexe Apr 24, 2020 •

edited

Loading