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

chore: impl default for identity #977

Merged
merged 4 commits into from
May 29, 2023
Merged

Conversation

ra0x3
Copy link

@ra0x3 ra0x3 commented May 22, 2023

Checklist

  • I have linked to any relevant issues.
  • I have updated the documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary labels.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

Description

  • I need to #[derive(Default)] for all types used by the indexer. Everything except Identity is working so far. I don't really care what it defaults to, just need a default for it.

@ra0x3 ra0x3 self-assigned this May 22, 2023
@iqdecay
Copy link
Contributor

iqdecay commented May 22, 2023

@FuelLabs/sdk-rust I don't know about you all but I'm wary of giving default a meaning for an Identity. On the other hand, I totally understand the need for the derivability.
Also we are already pretty much doing this for AssetId so maybe it makes sense, just have to document that default in our context means null.

@segfault-magnet segfault-magnet requested a review from a team May 23, 2023 09:02
@segfault-magnet
Copy link
Contributor

Also, we are already pretty much doing this for AssetId so maybe it makes sense, just have to document that default in our context means null.

Also, Address and ContractId are defaultable to 0.

I'd go ahead with the suggestion unless the rest of the team opposes it. Don't see somebody leaving the default value for the identity by mistake.

Copy link
Contributor

@hal3e hal3e left a comment

Choose a reason for hiding this comment

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

LGTM

@ra0x3 ra0x3 merged commit e1c5c1a into master May 29, 2023
32 checks passed
@ra0x3 ra0x3 deleted the rashad/impl-default-identity branch May 29, 2023 14:45
MujkicA pushed a commit that referenced this pull request Jun 5, 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.

None yet

5 participants