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.

Sign up for GitHub

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

Jump to bottom

Add api traits #363

Merged
merged 22 commits into from
Dec 13, 2022
Merged

Add api traits #363

merged 22 commits into from
Dec 13, 2022

Conversation

haerdib
Copy link
Contributor

@haerdib haerdib commented Dec 8, 2022 •
edited
Loading

Adds traits to the api and removes obsolete impl enforcements on the Api where possible (see #357)

closes #267

@haerdib haerdib self-assigned this Dec 8, 2022
@haerdib haerdib mentioned this pull request Dec 8, 2022
Closed
@haerdib haerdib added F7-enhancement Enhances an already existing functionality E2-breaksapi E1-breaksnothing and removed E2-breaksapi labels Dec 8, 2022
@haerdib haerdib marked this pull request as ready for review December 9, 2022 08:15
@haerdib haerdib requested a review from clangenb December 9, 2022 08:17
@haerdib
add traits
cd501f8 
add missing file

make clippy happier

cargo fmt

redo compose_extrinsic macro. Did not work before

fix doc

Bump secp256k1 from 0.24.0 to 0.24.2 (#364)

Bumps [secp256k1](https://github.com/rust-bitcoin/rust-secp256k1) from 0.24.0 to 0.24.2.
- [Release notes](https://github.com/rust-bitcoin/rust-secp256k1/releases)
- [Changelog](https://github.com/rust-bitcoin/rust-secp256k1/blob/secp256k1-0.24.2/CHANGELOG.md)
- [Commits](rust-bitcoin/rust-secp256k1@secp256k1-0.24.0...secp256k1-0.24.2)

---
updated-dependencies:
- dependency-name: secp256k1
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

add submitextrinsic trait

Added Polkadex in substrate-api-client readme (#365)
@haerdib haerdib marked this pull request as draft December 9, 2022 12:04
@haerdib haerdib force-pushed the bh/267-add-api-traits branch from e8c3419 to 7f9b278 Compare December 9, 2022 12:06
@haerdib haerdib removed the request for review from clangenb December 9, 2022 12:34
@haerdib haerdib marked this pull request as ready for review December 9, 2022 12:39
@haerdib haerdib requested review from clangenb and echevrier December 9, 2022 12:39
clangenb
clangenb reviewed Dec 12, 2022
View reviewed changes
Copy link
Collaborator

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Finally, the monolith has been separated!

However, I think that the separation is not really coherent with the substrate structure. What I propose to cleanly separate the stuff:

rpc_implementatsions/
   chain
   state
   constants
   frame_system
   pallet-balances
   pallet-transaction-payment

I do wonder if we should split up the subscriptions and put them into the respective domain, e.g.,

subscribe_finalized_head -> chain is this queries the chain RPC of the substrate node.

On the other hand, this would introduce a bit more complexity. I am unsure.

src/api/error.rs Outdated Show resolved Hide resolved
src/api/interfaces/frame_system.rs Outdated Show resolved Hide resolved
src/api/interfaces/generic_storage.rs Outdated Show resolved Hide resolved
src/api/interfaces/pallet_balances.rs Outdated Show resolved Hide resolved
src/api/interfaces/frame_system.rs Outdated Show resolved Hide resolved
haerdib
haerdib commented Dec 12, 2022
View reviewed changes
Copy link
Contributor Author

@haerdib haerdib left a comment

Choose a reason for hiding this comment

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

Moved all subscription, where possible, to the correspondent file. I think it makes more sense than to have all subscriptions in one file.

src/api/interfaces/subscription.rs Outdated
use sp_core::storage::StorageChangeSet;

// FIXME: This should rather be implemented directly on the
// Subscription return value, rather than the api. Or directly
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like how it's currently done. But changing this would break the interface. So that should be done in a different PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now, we only have events here. Maybe we can call it EventSubscription?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, actually, following the rust convention, that should rather be a verb noun. How about SubscribeEvents?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I agree that is even better, but the last thing would to also rename the file accordingly to subscribe_events :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

clangenb
clangenb reviewed Dec 12, 2022
View reviewed changes
Copy link
Collaborator

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Ah, looks much cleaner! A few minor remarks remain though. :)

examples/event_callback.rs Show resolved Hide resolved
src/api/interfaces/pallet_balances.rs Outdated Show resolved Hide resolved
src/api/interfaces/state.rs Outdated Show resolved Hide resolved
src/api/interfaces/frame_system.rs Outdated Show resolved Hide resolved
src/api/interfaces/subscription.rs Outdated
use sp_core::storage::StorageChangeSet;

// FIXME: This should rather be implemented directly on the
// Subscription return value, rather than the api. Or directly
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now, we only have events here. Maybe we can call it EventSubscription?

src/api/interfaces/subscription.rs Outdated Show resolved Hide resolved
@haerdib haerdib requested a review from clangenb December 12, 2022 14:59
clangenb
clangenb reviewed Dec 12, 2022
View reviewed changes
Copy link
Collaborator

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Looks much better now, I really like it. One last thing remains. :)

@haerdib haerdib requested a review from clangenb December 13, 2022 07:17
clangenb
clangenb approved these changes Dec 13, 2022
View reviewed changes
Copy link
Collaborator

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Whoopwhoop!

@haerdib haerdib merged commit ef25b8f into master Dec 13, 2022
@haerdib haerdib deleted the bh/267-add-api-traits branch December 13, 2022 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clangenb clangenb clangenb approved these changes

@echevrier echevrier Awaiting requested review from echevrier

Assignees

@haerdib haerdib

Labels
E1-breaksnothing F7-enhancement Enhances an already existing functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Abstract api-client main features behind a trait
2 participants
@haerdib @clangenb