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: initial feature set #1

Merged
merged 8 commits into from
Nov 22, 2023
Merged

feat: initial feature set #1

merged 8 commits into from
Nov 22, 2023

Conversation

ntn-x2
Copy link
Member

@ntn-x2 ntn-x2 commented Nov 15, 2023

Initial feature set. Integration tests will follow in a different PR. Code has been widely tested in many different scenarios via KILTprotocol/kilt-did-utilities#13, and ported here and adapted accordingly.

@ntn-x2 ntn-x2 self-assigned this Nov 15, 2023
@ntn-x2 ntn-x2 changed the title chore: initial setup feat: initial feature set Nov 15, 2023
@ntn-x2 ntn-x2 marked this pull request as ready for review November 15, 2023 15:02
Copy link
Contributor

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

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

Holy moly, this is some complex stuff! I can mainly advise on the use of the polkadot api and on the typescript setup I think, both of which look good.
There are some features of the polkadot api you can make more use of to make the code a little less convoluted; I left suggestions that show that in some examples.
One more general thing you could look into is if it may be cool to have some functions available as custom derives as well, which could be quite elegant. Also derives work for both the rxjs-based api and the promise-based api (the latter of which we have always used), so may integrate better with other people's approaches. Unfortunately derives do not work where you need api connections to two different chains as far as I know; this is what CAPI was meant to solve. Really bummed out that they killed it, because this project right here is exactly the kind of thing you would want to be using it for...

tsconfig.cjs.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Show resolved Hide resolved
Comment on lines +75 to +80
const relayParentBlockNumber = await providerApi
.at(providerBlockHash)
.then((api) => api.query.parachainSystem.lastRelayChainBlockNumber())
const relayParentBlockHash = await relayApi.rpc.chain.getBlockHash(
relayParentBlockNumber,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

this would be so much nicer to implement on capi 😢

src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
@ntn-x2 ntn-x2 merged commit c92aa09 into main Nov 22, 2023
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.

2 participants