-
Notifications
You must be signed in to change notification settings - Fork 151
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 endpoints #504
Add endpoints #504
Conversation
src/services/paras/ParasService.ts
Outdated
* } | ||
* @param hash `BlockHash` to make call at | ||
* @param paraId ID of para to get crowdloan info for | ||
* @returns crowdloan information for paradId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this, We don't document any of the @returns
in any other services, we could get rid of this. Up to you though.
src/services/paras/ParasService.ts
Outdated
* } | ||
* @param hash `BlockHash` to make call at | ||
* @param includeFundInfo wether or not to include `FundInfo` for every crowdloan | ||
* @returns list of all crowdloans |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before. Maybe we could remove these @returns
and write doc info above the @param
notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is really that important more so an observation of continuity between all services.
@@ -45,51 +71,44 @@ export class ParasService extends AbstractService { | |||
return { | |||
at, | |||
fundInfo, | |||
// TOOD would it make snese to merge the below derived info into `fundInfo` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Little confused on this could you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically should we add leasePeriods
as a key within the fundInfo
object?
blockNumber | ||
).toNumber(); | ||
|
||
leasesFormatted = leases.reduce((acc, curLeaseOpt, idx) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very Nice!
leasesFormatted = null; | ||
} | ||
|
||
let onboardingAs: ParaType | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make an IOption<T>
for undefined's?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be open to it, but would want to call it something else.
My only hold up though are that undefined
fn params and interface properties already have the ?
operator which is normally preferred over having a union type with undefined.
blockNumber = number.unwrap(); | ||
|
||
currentLeaseHolders = leaseEntries | ||
.filter(([_k, leases]) => leases[0].isSome) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the use of isSome
, I think it has the ability to also optimize code in a lot of other service, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup - just to clarify though, this is .isSome
is a property on the polkadot-js Option
enum which extends Codec
- which is different than the IOption
type alias we have defined within this repo.
|
||
const parasPromises = paraLifecycles.map(async ([k, paraLifeCycle]) => { | ||
const paraId = k.args[0]; | ||
let onboardingAs: ParaType | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here could we have a Option for undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, little grammar nit pics.
Also comments about the @returns
, up to you if you want them there or not.
Co-authored-by: Tarik Gul <47201679+TarikGul@users.noreply.github.com>
Adds endpoints:
paras/
: list of all registered parachains and their lifecycle stage./:paraId/lease-info
: current and future lease information for a para./:paraId/crowdloan-info
: info on a para's current crowdloan./leases/current
: list of all lease holders for the current lease period and general information about the current lease period./auctions/current
: information on the current auction.Future PRs:
crowdloan-info
&lease-info
into a single endpoint that just has all the plo info that is specific to a chain.