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

Pull in observable API from UI #175

Merged
merged 21 commits into from
Oct 1, 2018
Merged

Pull in observable API from UI #175

merged 21 commits into from
Oct 1, 2018

Conversation

jacogr
Copy link
Member

@jacogr jacogr commented Sep 28, 2018

Related #161

@jacogr jacogr added the WIP Work in Progress label Sep 28, 2018
@jacogr
Copy link
Member Author

jacogr commented Sep 30, 2018

Ok, pretty happy (?) with where it as a phase I. I had to limit the intent a bit and stuck with the existing naming instead of (as a first try), renaming everything along the way as well. So it basically just imports the new names and types.

As it stands, going to just use it as-is in the UI and see what pops up. Once working as expected, can marge and actually use the package. (Just from a speed of use perspective, sticking with the in-UI updated version with these large number of changes is a bit more prudent)

So as a first merge, the usable important bits are actually the small changes to codec - the api-observable is not quite intended for use yet, nor it is really mean to be.

@jacogr jacogr removed the WIP Work in Progress label Sep 30, 2018
@@ -285,9 +287,9 @@ export default class RuntimeMetadata extends Struct {

// We receive this as an Array<number> in the JSON output from the Node. Convert
// to u8a and use the fromU8a to do the actual parsing
fromJSON (input: Array<number>): RuntimeMetadata {
fromJSON (input: Uint8Array | string | Array<number>): RuntimeMetadata {
Copy link
Member Author

Choose a reason for hiding this comment

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

The actual JSON has changed, it now comes in not as an Array<number>, but rather as a hex string. Cater for all.

@@ -82,6 +84,10 @@ export default class Struct <
}

fromJSON (input: any): Struct<S, T, V, E> {
if (isHex(input)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Allows hex inputs, which gets treated as u8a


// helpers from BN, this would be great as a "don't do this", i.e. extending properly
// from BN. Underlying these will always return BN (unless it is compare checks)
add (other: UInt | BN | number): BN {
Copy link
Member Author

Choose a reason for hiding this comment

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

Some helpers as a stop-gap, see comment of the Base interface PR.

// nn nn nn 11 [ / zz zz zz zz ]{4 + n}
//
// Note: we use *LOW BITS* of the LSB in LE encoding to encode the 2 bit key.
export default class Compact {
Copy link
Member Author

Choose a reason for hiding this comment

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

We pulled this encoding (basically just the encode/decode functions) from Length - parity-codec is looking to use this for all numbers, so make it share-able as "utility" functions. (Naming for the encoding now also aligns with what we have in parity-codec)

constructor (value: AnyNumber = new BN(0)) {
private _bitLength: BitLength;

constructor (value: AnyNumber = new BN(0), bitLength: BitLength = 32) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Bit out of scope for this PR, so not changing things here... but since we have pulled the encoding logic into Compact, we would actually get rid of this use - rather, just call Compact.encode() with the length directly in the variable length uses instead of replying on Length. (Which as per the FIXME has always been a bit of a funny step-child)

const defaultMapFn = (result: any): any =>
result;

export default class ApiBase {
Copy link
Member Author

Choose a reason for hiding this comment

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

Split out the implementation from the UI -

  • Base - just the basics, i.e. a nice wrapper for raw calls and storage
  • Calls - Anything that maps to an actual call on the API, i.e. getBlock
  • Queries - Anything that just simply wraps a storage query
  • Combined - Multiple observables combined into a single result

);
}

stakingFreeBalanceOf = (address: AccountId | string): Observable<Balance | undefined> => {
Copy link
Member Author

Choose a reason for hiding this comment

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

These need to be renamed - kept it as-is for the initial PR, i.e. just takin what is there and moving it to the correct storage area. (Actually started off renaming all, but in reality would first need to get the UI working and then start renaming)

Copy link
Contributor

@amaury1093 amaury1093 Oct 1, 2018

Choose a reason for hiding this comment

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

Ok, makes sense.

Edit: was wrt your previous comment about not renaming for now.

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Since this is the most user-facing library we'll have, comments are imo mandatory on all methods. But can be left for a later PR, when everything is more settled down.

[storage.balances.transactionByteFee],
[storage.balances.creationFee],
[storage.balances.existentialDeposit],
[storage.balances.transferFee ]
Copy link
Contributor

Choose a reason for hiding this comment

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

extra whitespace

);
}

stakingFreeBalanceOf = (address: AccountId | string): Observable<Balance | undefined> => {
Copy link
Contributor

@amaury1093 amaury1093 Oct 1, 2018

Choose a reason for hiding this comment

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

Ok, makes sense.

Edit: was wrt your previous comment about not renaming for now.

// of the ISC license. See the LICENSE file for details.

import BN from 'bn.js';
import { AccountId, Balance, BlockNumber, PropIndex, Proposal, ReferendumIndex, VoteThreshold } from '@polkadot/api-codec/index';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is /index needed here? I guess for end-users it's also needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Habit. In the UI (I think) the tsconfig path mapping is a bit wonky with some packages. (Just the regex not being quite as good as it should be). Here it is actually not needed. (Looking at tsconfig, quite simple here). For end-users it is never required.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, just checked, it still is needed - I think it is a tsconfig regex change, i.e. changing it to @polkadot/api-codec and it complains.

@jacogr
Copy link
Member Author

jacogr commented Oct 1, 2018

100% with you on the documentation - and something else sorely missing here is the testing. (But first get it working and named properly.)

@jacogr jacogr merged commit 782bb80 into master Oct 1, 2018
@jacogr jacogr deleted the jg-api-observable branch October 1, 2018 09:22
@polkadot-js-bot
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Jun 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants