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

Hf20 testnet appbase #4

Merged
merged 11 commits into from
Sep 20, 2018
Merged

Hf20 testnet appbase #4

merged 11 commits into from
Sep 20, 2018

Conversation

roadscape
Copy link

Replaces #2 and #3 -- plus, it removes the account creation endpoint since the new testnet doesn't have one (yet).

$ yarn test
yarn test v0.20.4
$ make ci-test
tslint -p tsconfig.json -c tslint.json
nyc -r lcov -e .ts -i ts-node/register mocha --exit --reporter tap --require ts-node/register test/*.ts
1..80
ok 1 asset should create from string
ok 2 asset should convert to string
ok 3 asset should add and subtract
ok 4 asset should max and min
ok 5 asset should throw on invalid values
ok 6 asset should parse price
ok 7 asset should get vesting share price
ok 8 asset should convert price
ok 9 blockchain should yield blocks
ok 10 blockchain should stream blocks
ok 11 blockchain should yield operations
ok 12 blockchain should stream operations
ok 13 blockchain should yield latest blocks
ok 14 blockchain should handle errors on stream
ok 15 blockchain should get block number stream
ok 16 blockchain should get current block header
ok 17 broadcast should broadcast
ok 18 broadcast should handle concurrent broadcasts
ok 19 client should make rpc call
ok 20 client should handle rpc errors
ok 21 client should format rpc errors
ok 22 client should retry and timeout
ok 23 crypto should decode public keys
ok 24 crypto should decode private keys
ok 25 crypto should create public from private
ok 26 crypto should handle prefixed keys
ok 27 crypto should conceal private key when inspecting
ok 28 crypto should sign and verify
ok 29 crypto should de/encode signatures
ok 30 crypto should recover pubkey from signatures
ok 31 crypto should create key from login
ok 32 crypto should sign and verify transaction
ok 33 crypto should handle serialization errors
ok 34 database api getDynamicGlobalProperties
ok 35 database api getConfig
ok 36 database api getBlockHeader
ok 37 database api getBlock
ok 38 database api getOperations
ok 39 database api getDiscussions
ok 40 database api getTransaction
ok 41 database api getChainProperties
ok 42 database api getCurrentMedianHistoryPrice
ok 43 database api getVestingDelegations
ok 44 database api verifyAuthority
ok 45 misc iteratorStream should handle backpressure
ok 46 misc iteratorStream should handle errors
ok 47 operations should delegate vesting shares
ok 48 operations should send custom
ok 49 operations should send custom json
ok 50 operations should transfer steem
not ok 51 operations should create account and post with options
  RPCError: Account: ds-hecdrlgutrgt needs 1165896 RC. Please wait to transact, or power up STEEM.
      at Client.<anonymous> (src/client.ts:53:119)
      at Generator.next (<anonymous>)
      at fulfilled (src/client.ts:34:13159)
      at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:160:7)
ok 52 operations should update account
ok 53 operations should create account custom auths
ok 54 operations should create account and calculate fees
ok 55 operations should change recovery account
ok 56 operations should report overproduction
ok 57 serializers Asset
ok 58 serializers Array::String
ok 59 serializers Array::UInt16
ok 60 serializers Boolean
ok 61 serializers Date
ok 62 serializers FlatMap::UInt8::UInt8
ok 63 serializers FlatMap::String::Date
ok 64 serializers Int8
ok 65 serializers Int16
ok 66 serializers Int32
ok 67 serializers Int64
ok 68 serializers UInt8
ok 69 serializers UInt16
ok 70 serializers UInt32
ok 71 serializers UInt64
ok 72 serializers Optional::UInt8
ok 73 serializers Operation
ok 74 serializers Transaction
ok 75 serializers String
ok 76 serializers Price
ok 77 serializers PublicKey
ok 78 serializers Binary
ok 79 serializers Void
ok 80 serializers Invalid Operations
# tests 80
# pass 79
# fail 1
make: *** [ci-test] Error 1

FYI: test 51 can also fail with

not ok 51 operations should create account and post with options
  RPCError: Missing Posting Authority ds-g5kgedaf02tn
      at Client.<anonymous> (src/client.ts:53:119)
      at Generator.next (<anonymous>)
      at fulfilled (src/client.ts:34:13159)
      at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:160:7)

This is due to insufficient RC when processing the 2nd op (comment_options)

@roadscape
Copy link
Author

TODO:

  • audit usage of STEEM and TESTS symbols
  • discourage use of createAccount helper -- assert it's only called w/in mocha?
    • unclear if used by clients or only as test helper. perhaps rename new implementation to claimAndCreateAccount, remove delegation logic, and ensure old method simply throws

opts.chainId = '79276aea5d4877d9a25892eaa01b0adf019d3e5cb12a97478df3298ccdd01673'
return new Client('https://testnet.steem.vc', opts)

opts.addressPrefix = 'TST'

Choose a reason for hiding this comment

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

These assignments could be an issue. addressPrefix & chainId are not guaranteed on any testnet, including testnet.steemitdev.com; in fact there are explicit stories and direction on the dev portal about the chain id changing to something derived from the current git-hash.

Steemit maintains a live testnet to which you can connect. In the near future, we expect the chain id to update with every code change.

We need to have these update only-if-not-set.

Copy link
Author

Choose a reason for hiding this comment

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

Looking for smallest set of changes in this PR, let's revisit this later. Still need to figure out how we'll handle it in the other clients too.

Copy link

@relativityboy relativityboy Sep 20, 2018

Choose a reason for hiding this comment

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

Still need to figure out how we'll handle it in the other clients too.

I agree, we need to address this need in other clients as well, but that's not relevant here.

Added issue #5 to track our needs, and approving to keep the flow going.

Copy link

@relativityboy relativityboy left a comment

Choose a reason for hiding this comment

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

The d in dsteem...

@@ -249,6 +240,18 @@ export interface ClaimRewardBalanceOperation extends Operation {
}
}

export interface ClaimAccountOperation extends Operation {

Choose a reason for hiding this comment

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

Operation needs to be documented.

@@ -288,6 +291,23 @@ export interface ConvertOperation extends Operation {
}
}

export interface CreateClaimedAccountOperation extends Operation {

Choose a reason for hiding this comment

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

Operation needs to be documented.

extensions: any[] // extensions_type
}
}

export interface CustomOperation extends Operation {

Choose a reason for hiding this comment

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

Operation needs to be documented.

gl2748
gl2748 previously approved these changes Sep 19, 2018
Copy link

@gl2748 gl2748 left a comment

Choose a reason for hiding this comment

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

LGTM

@relativityboy relativityboy dismissed gl2748’s stale review September 20, 2018 03:48

PR is currently pending changes. The code in its current state would resist currently planned changes in the testnet, increasing tech-debt. There are already plans to correct in this PR.

opts.chainId = '79276aea5d4877d9a25892eaa01b0adf019d3e5cb12a97478df3298ccdd01673'
return new Client('https://testnet.steem.vc', opts)

opts.addressPrefix = 'TST'
Copy link

@relativityboy relativityboy Sep 20, 2018

Choose a reason for hiding this comment

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

Still need to figure out how we'll handle it in the other clients too.

I agree, we need to address this need in other clients as well, but that's not relevant here.

Added issue #5 to track our needs, and approving to keep the flow going.

@roadscape roadscape merged commit 922379c into master Sep 20, 2018
@roadscape roadscape deleted the hf20-testnet-appbase branch September 20, 2018 20:38
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