refactor: swap ID from u64 to `SizedAsciiString<64>` #1217

ra0x3 · 2023-08-07T19:38:59Z

Thanks for opening a PR with the Fuel Indexer project, please review the Checklist below and ensure you've completed all of the necessary steps to make this PR review as painless as possible.

Checklist

Ensure your top-level commit message is inline with our contributor guidelines.
Please add proper labels.
If there is an issue associated with this PR, please link the issue (right-hand sidebar)
If there is not an issue associated with this PR, add this PR to the "Fuel Indexer" project (right-hand sidebar)
Please allow Codeowners at least 24 hours to do a first-pass review.
Please add thorougly detailed testing steps below.
Please keep your Changelog message short and sweet.

Description

I noticed quite a bit of weirdness whilst working on this - so we won't be cutting 0.20 immediately until we add some more (relevant, non-redundant, useful) testing
PR does a few things
- Does not optimize usage of .clone() Don't use too much .clone() on ID types #1278
- Refactors the project to use SizedAsciiString<64> IDs instead of u64 IDs
  - With SizedAsciiString<64> aliased as UID
- Removes simple-wasm indexer
  - It wasn't testing anything we weren't already testing
- Also includes more work from Optimize and condense E2E tests #1194
  - We don't use any other DB anymore so removed "postgres" suffix from test modules
  - Moves the fuel-indexer-test indexer from fuel-indexer-tests/components/indices/ to fuel-indexer-tests/indexers
    - simple-wasm too
  - Removes redundant tests (database) that have not detected much breakage in the past
  - Removes the graphql_schema test module
    - Our GraphQL layer is gonna get an entire makeover per Integrate web server, GraphQL layer, and data layer #1145
      - Grantees are using the SQL endpoint
- Updates our trybuild tests (which have not been running on CI :/ )
  - Re-enables these on CI
- Moves IndexMetadataEntity to fuel_indexer_types
  - And makes fuel_indexer_lib dependent on fuel_indexer_types
    - fuel_indexer_types is now the base crate (previously was fuel_indexer_lib)

Testing steps

As this was a very low-level/granular change, CI for everything should pass
Start your indexer service on beta-4 cargo run --bin fuel-indexer -- run --run-migrations --fuel-node-host beta-4.fuel.network --fuel-node-port 80
Deploy the default indexer forc index new indexer-test --namespace fuellabs && cd indexer-test && forc index deploy
Let this index to around block 250K, you should have no warnings for constraint errors

Changelog

refactor: swap ID from u64 to SizedAsciiString<64>

@xgreenx

* chore: upgrade to rust 1.71 * rebase origin/develop * Bump dependencies across workspace * Adjust fuel types in fuel-indexer-types * Remove temp workarounds in indexer macro * Bring in fuel-tx v0.35.3, thank you @xgreenx * Fix most of explorer; optimize data decoding in indexer macro * Clippy fixes * Fix indexing_postgres e2e tests * Manually convert contract IDs in indexer codegen * Fix final type errors * Upgrade fuel-indexer-tests * Add missing fields to client schema * Finish the upgrade...hopefully * Upgrade to fuels-rs v0.45 * ra0x3 feedback * Fix CI failures --------- Co-authored-by: Rashad Alston <rashad@Rashads-Air.lan>

* First pass; combine non-output receipt tests into one * Combine explorer-related tests * Combine tests that hit /ping * Combine receipt tests again * Finish optimization of e2e::indexing_postgres * Remove unused replace-indexer tests from indexing_postgres * Remove outdated, unused test from integration::service * Condense GraphQL API E2E tests * Use get_or_create() in test indexer * Remove IDs checks in E2E tests * Cleanup formatting and folder structure * Move http_client into WebTestComponents * Upgrade Sway version * cargo fmt * Adjust EXPECTED_CONTRACT_ID * Upgrade fuels-rs to v0.45.1

@xgreenx

* chore: upgrade to rust 1.71 * rebase origin/develop * Bump dependencies across workspace * Adjust fuel types in fuel-indexer-types * Remove temp workarounds in indexer macro * Bring in fuel-tx v0.35.3, thank you @xgreenx * Fix most of explorer; optimize data decoding in indexer macro * Clippy fixes * Fix indexing_postgres e2e tests * Manually convert contract IDs in indexer codegen * Fix final type errors * Upgrade fuel-indexer-tests * Add missing fields to client schema * Finish the upgrade...hopefully * Upgrade to fuels-rs v0.45 * ra0x3 feedback * Fix CI failures --------- Co-authored-by: Rashad Alston <rashad@Rashads-Air.lan>

ra0x3 · 2023-08-16T21:15:28Z

packages/fuel-indexer-types/src/lib.rs

 }

-#[cfg(test)]


Not even sure we ever used these conversions

packages/fuel-indexer-tests/tests/graphql_server.rs

deekerno

Running through your testing steps now; left a few questions/suggestions in the meantime.

packages/fuel-indexer-tests/tests/graphql_server.rs

deekerno · 2023-08-17T19:37:03Z

packages/fuel-indexer-plugin/src/wasm.rs

        unsafe {
-            let buf = id.to_le_bytes();
-            let mut buflen = (buf.len() as u32).to_le_bytes();
+            let buff = bincode::serialize(&id.to_string()).unwrap();


I think we can remove this .to_string() call here as SizedAsciiString provides an impl for Serialize: https://github.com/FuelLabs/fuels-rs/blob/df21541b679bb90a1ee3e54f34a67b7037b40be7/packages/fuels-core/src/types/core/sized_ascii_string.rs#L162-L169.

Additionally, can we replace the .unwrap() with an .expect()?

packages/fuel-indexer/src/ffi.rs

lostman · 2023-08-17T20:00:50Z

packages/fuel-indexer-api-server/src/uses.rs

+        {
+            error!("Failed to send ServiceRequest::Reload: {e:?}");
+            return Err(e.into());
+        }


I wonder whether there is a better way of handling this pattern.

The error returned by ? is generally logged somewhere up. We have a couple of error messages that repeat this way.

The advantage of the error! macro is the source location attribution.

Perhaps there is some way to have both concise code and precise location.

@lostman

I agree it seems a bit...repetitive?

I think the main reason I bubbled things up in this way (and in executor/service) was to be almost annoyingly explicit with the path of the error (where we can trace the error's path just via looking at the logs)

I'm open to better ways to accomplish this

packages/fuel-indexer-tests/indexers/fuel-indexer-test/fuel_indexer_test.yaml

ra0x3 added chore work in progress labels Aug 7, 2023

ra0x3 self-assigned this Aug 7, 2023

ra0x3 requested a review from deekerno as a code owner August 7, 2023 19:39

ra0x3 marked this pull request as draft August 7, 2023 19:39

This was linked to issues Aug 8, 2023

Refactor ID from u64 to SizedAsciiString #1208

Closed

How to handle case where two different TypeDefinitions derive the same ID? #1093

Closed

ra0x3 added bug Something isn't working blocked This change is currently blocked by a linked issue and removed chore labels Aug 8, 2023

ra0x3 changed the base branch from develop to feature/beta-4-release August 11, 2023 14:33

ra0x3 force-pushed the rashad/1208-refactor-ID-to-string branch 2 times, most recently from 95a538b to 8e907a4 Compare August 15, 2023 14:49

ra0x3 force-pushed the feature/beta-4-release branch 2 times, most recently from 300f3ff to 2dc1d4d Compare August 15, 2023 16:42

ra0x3 force-pushed the rashad/1208-refactor-ID-to-string branch from 83a25cd to 9130eef Compare August 15, 2023 18:32

Base automatically changed from feature/beta-4-release to develop August 15, 2023 20:47

ra0x3 force-pushed the rashad/1208-refactor-ID-to-string branch from 9130eef to 8a239a0 Compare August 15, 2023 22:16

ra0x3 removed the blocked This change is currently blocked by a linked issue label Aug 15, 2023

ra0x3 force-pushed the rashad/1208-refactor-ID-to-string branch from e3658eb to 45c4a79 Compare August 16, 2023 14:16

ra0x3 changed the title ~~refactor: swap ID from u64 to String~~ refactor: swap ID from u64 to SizedAsciiString<64> Aug 16, 2023

deekerno and others added 9 commits August 16, 2023 15:19

enhancement: add support for Mint and Burn receipts (#1259)

663a7ba

refactor: swap ID from u64 to String

e62c7d0

update

d3f4dae

stuck on sizedascii serde

b1870a9

updates still pending fuels-rs update

6211a79

rebase origin/feature/beta-4-release

acf08ba

Rashad Alston added 8 commits August 16, 2023 15:19

fix bad rebase

53110c5

swap sizedascii to uid

b70a4a7

add UID as scalar

4862500

updates

87ea6ad

rebase origin/develop

75b9397

get explorer working

bb59508

fix the tests

813bdf3

rebase origin/develop

adaf55b

ra0x3 force-pushed the rashad/1208-refactor-ID-to-string branch from e8d5044 to adaf55b Compare August 16, 2023 19:19

update comment

f963e91

ra0x3 mentioned this pull request Aug 16, 2023

Don't use too much .clone() on ID types #1278

Closed

updates

16e04be

ra0x3 added breaking This is a change that will break some component of the project once merged to origin/develop and removed work in progress labels Aug 16, 2023

ra0x3 marked this pull request as ready for review August 16, 2023 20:12

ra0x3 requested a review from lostman as a code owner August 16, 2023 20:12

Rashad Alston added 3 commits August 16, 2023 16:17

update tests

da182a5

update tests again

7a5884e

test fixes

9db959f

ra0x3 commented Aug 16, 2023

View reviewed changes

packages/fuel-indexer-types/src/lib.rs

}

#[cfg(test)]

Copy link

Contributor Author

ra0x3 Aug 16, 2023

Choose a reason for hiding this comment

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

Not even sure we ever used these conversions

ra0x3 commented Aug 16, 2023

View reviewed changes

packages/fuel-indexer-tests/tests/graphql_server.rs Show resolved Hide resolved

add more clarity to schema mismatch

1b690d8

ra0x3 force-pushed the rashad/1208-refactor-ID-to-string branch from 74194ae to 1b690d8 Compare August 16, 2023 21:59

deekerno suggested changes Aug 17, 2023

View reviewed changes

deekerno feedback

fdb5716

lostman approved these changes Aug 17, 2023

View reviewed changes

ra0x3 requested a review from deekerno August 17, 2023 20:24

deekerno approved these changes Aug 17, 2023

View reviewed changes

ra0x3 merged commit 45e602b into develop Aug 17, 2023

ra0x3 deleted the rashad/1208-refactor-ID-to-string branch August 17, 2023 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: swap ID from u64 to `SizedAsciiString<64>` #1217

refactor: swap ID from u64 to `SizedAsciiString<64>` #1217

ra0x3 commented Aug 7, 2023 •

edited

Loading

ra0x3 Aug 16, 2023

deekerno left a comment

deekerno Aug 17, 2023

lostman Aug 17, 2023

ra0x3 Aug 17, 2023

refactor: swap ID from u64 to SizedAsciiString<64> #1217

refactor: swap ID from u64 to SizedAsciiString<64> #1217

Conversation

ra0x3 commented Aug 7, 2023 • edited Loading

Checklist

Description

Testing steps

Changelog

ra0x3 Aug 16, 2023

Choose a reason for hiding this comment

deekerno left a comment

Choose a reason for hiding this comment

deekerno Aug 17, 2023

Choose a reason for hiding this comment

lostman Aug 17, 2023

Choose a reason for hiding this comment

ra0x3 Aug 17, 2023

Choose a reason for hiding this comment

refactor: swap ID from u64 to `SizedAsciiString<64>` #1217

refactor: swap ID from u64 to `SizedAsciiString<64>` #1217

ra0x3 commented Aug 7, 2023 •

edited

Loading