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!(crates): remove iota name service #1722

Merged
merged 8 commits into from
Aug 26, 2024

Conversation

Thoralf-M
Copy link
Member

@Thoralf-M Thoralf-M commented Aug 9, 2024

Description of change

Removed all iota name service related things from crates/* as we won't have a name service for now and later it might look a bit different. Remaining things will be done in another PR #1611
Graphql schema was updated with cargo run generate-schema > schema/current_progress_schema.graphql

Links to any relevant issues

Fixes #1607

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How the change has been tested

cargo check

@Thoralf-M Thoralf-M added the dev-tools Issues related to the Developer Tools Team label Aug 9, 2024
@Thoralf-M Thoralf-M requested review from a team as code owners August 9, 2024 09:25
@Thoralf-M Thoralf-M force-pushed the dev-tools/remove-iota-name-service branch from efb06a1 to a9cafb9 Compare August 9, 2024 09:37
@Thoralf-M Thoralf-M requested review from a team as code owners August 9, 2024 09:37
@thibault-martinez
Copy link
Member

warning: method `as_slice` is never used
  --> crates/iota-graphql-rpc/src/types/iota_address.rs:52:12
   |
43 | impl IotaAddress {
   | ---------------- method in this implementation
...
52 |     pub fn as_slice(&self) -> &[u8] {
   |            ^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

Copy link
Member

@thibault-martinez thibault-martinez left a comment

Choose a reason for hiding this comment

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

There is a mention to NAME_SERVICE in crates/iota-graphql-rpc/tests/snapshots/snapshot_tests__schema_sdl_export.snap, do you know if we should remove?

@Thoralf-M
Copy link
Member Author

@thibault-martinez good catch, removed by running cargo -q run --example generate-json-rpc-spec -- record

@thibault-martinez
Copy link
Member

thibault-martinez commented Aug 9, 2024

Last few mentions of Iotans in

  • crates/iota-graphql-rpc/docs/examples.md
  • crates/iota-graphql-rpc/examples/object_connection/object_connection.graphql
  • docs/content/references/ts-sdk/dapp-kit/rpc-hooks.mdx

@Thoralf-M
Copy link
Member Author

docs/content/references/ts-sdk/dapp-kit/rpc-hooks.mdx

is related to the ts-sdk and should be done in #1611 I think

Copy link
Contributor

@kodemartin kodemartin left a comment

Choose a reason for hiding this comment

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

Hey @Thoralf-M, looks good in general. Left a few suggestions/questions of non-critical nature.

Comment on lines 1 to 20
{
objects {
nodes {
version
digest
storageRebate
previousTransactionBlock {
digest
sender { defaultIotansName }
gasInput {
gasPrice
gasBudget
}
}
}
pageInfo {
endCursor
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to remove the complete example. Suffice to refactor the sender field query like so

sender { address }

instead of

sender { defaultIotansName }

@@ -68,8 +67,6 @@ pub(crate) fn graphql_error_at_pos(
pub enum Error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest making this #[non_exhaustive] to allow readding the variant without this being a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Do you think we should overall make all public enums/errors #[non_exhaustive]?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would be lazy about it. That is I would limit this to whatever we touch and makes sense to make it so.

@@ -130,9 +129,6 @@ pub enum IndexerError {

#[error("Indexer failed to send item to channel with error: `{0}`")]
MpscChannelError(String),

#[error(transparent)]
NameServiceError(#[from] NameServiceError),
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this enum #[non_exhaustive] as well.

@@ -76,9 +76,6 @@ pub enum Error {

#[error("Unsupported Feature: {0}")]
UnsupportedFeature(String),

#[error("transparent")]
NameServiceError(#[from] NameServiceError),
Copy link
Contributor

Choose a reason for hiding this comment

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

As for the other error enums, this can be more future-proof if we render it as #[non_exhaustive]

crates/iota-json-rpc/src/indexer_api.rs Show resolved Hide resolved
restore object_connection.graphql
Copy link
Contributor

@kodemartin kodemartin left a comment

Choose a reason for hiding this comment

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

lgtm 🌹

Copy link
Contributor

⚠️ 🦋 Changesets Warning: This PR has changes to public npm packages, but does not contain a changeset. You can create a changeset easily by running pnpm changeset in the root of the IOTA repo, and following the prompts. If your change does not need a changeset (e.g. a documentation-only change), you can ignore this message. This warning will be removed when a changeset is added to this pull request.

Learn more about Changesets.

@Thoralf-M Thoralf-M merged commit 73dfabf into update-rpc Aug 26, 2024
66 of 70 checks passed
@Thoralf-M Thoralf-M deleted the dev-tools/remove-iota-name-service branch August 26, 2024 09:06
alexsporn pushed a commit that referenced this pull request Sep 6, 2024
* feat!(crates): remove iota name service

* Update openrpc.json

* Add #[cfg(test)] for method that's only used in tests

* Update snapshot_tests__schema_sdl_export.snap

* Update snapshot_tests__populated_genesis_snapshot_matches-2.snap

* Remove object_connection

* Make error enums #[non_exhaustive];
restore object_connection.graphql
marc2332 pushed a commit that referenced this pull request Sep 10, 2024
* feat!(crates): remove iota name service

* Update openrpc.json

* Add #[cfg(test)] for method that's only used in tests

* Update snapshot_tests__schema_sdl_export.snap

* Update snapshot_tests__populated_genesis_snapshot_matches-2.snap

* Remove object_connection

* Make error enums #[non_exhaustive];
restore object_connection.graphql
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev-tools Issues related to the Developer Tools Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task: (SC-Platform)]: remove iota name service
4 participants