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

Introduce AccountId version wrapper #1058

Merged
merged 31 commits into from
Jan 13, 2025

Conversation

PhilippGackstatter
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter commented Jan 10, 2025

The changes in this PR are:

  • Introduce AccountId version wrapper for AccountId and AccountIdPrefix. Both seemed necessary since prefix is also specific to V0.
  • Refactor AccountIdVersion from a struct into an enum as it makes it possible to exhaustively match on all versions.
  • Move AccountType, AccountStorageMode and AccountIdVersion to separate modules. Nothing was changed about these, so the code doesn't have to be reviewed again.

This approach should generally allow to introduce another AccountIdV1 without breaking APIs, except if:

  • its byte format is not [u8; 15]. This seems like something we at least won't increase since that would break the metadata encoding.
  • If we add a new AccountType or AccountStorageMode in the future it would be breaking. A wrapper for these doesn't seem quite right though. We could simply make AccountStorageMode and AccountType non-exhaustive, so at least the API would not be broken if we add new variants. However, that obviously breaks exhaustive matching which is very nice to have. And perhaps it's okay if these kinds of additions are a breaking change on the API level.

Should the inner version's methods be publicly exposed or should we make them private? Some code may want to work with a specific ID version so it seems best to keep them public.

Some deduplication of documentation would be nice. Perhaps the V0 methods should point to the wrapper methods unless they're different (like if we have a new version in the future), e.g. on AccountIdV0:

/// See [`AccountId::account_type`].
pub const fn account_type(&self) -> AccountType { ... }

part of #1046.

@PhilippGackstatter PhilippGackstatter marked this pull request as ready for review January 10, 2025 14:22
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left some comments inline - most are minor. Once these are addressed, we can merge.

its byte format is not [u8; 15]. This seems like something we at least won't increase since that would break the metadata encoding.

Yes, I think we can assume that IDs will remain 15 bytes in the foreseeable future.

  • If we add a new AccountType or AccountStorageMode in the future it would be breaking. A wrapper for these doesn't seem quite right though. We could simply make AccountStorageMode and AccountType non-exhaustive, so at least the API would not be broken if we add new variants. However, that obviously breaks exhaustive matching which is very nice to have. And perhaps it's okay if these kinds of additions are a breaking change on the API level.

I would keep them exhaustive.

Should the inner version's methods be publicly exposed or should we make them private? Some code may want to work with a specific ID version so it seems best to keep them public.

Yes, I think that's fine.

Some deduplication of documentation would be nice. Perhaps the V0 methods should point to the wrapper methods unless they're different (like if we have a new version in the future), e.g. on AccountIdV0:

/// See [`AccountId::account_type`].
pub const fn account_type(&self) -> AccountType { ... }

I'm fine with this - though some of the comments I left are about making sure that these types of references leading to the actual docs (rather than another reference).

objects/src/assets/nonfungible.rs Outdated Show resolved Hide resolved
objects/src/accounts/account_id/id.rs Outdated Show resolved Hide resolved
objects/src/accounts/account_id/id.rs Outdated Show resolved Hide resolved
/// Note that the `anchor` must correspond to a valid block in the chain for the ID to be deemed
/// valid during creation.
///
/// See the documentation of the [`AccountId`] for more details on the generation.
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation of account ID just refers us to docs of specific versions. Should we refer to specific versions here and avoid this extra potential "hop"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, thanks for pointing this out.

I resolved it by moving the main account ID documentation to AccountId directly. If we add another version in the future, it will most likely differ only in some details, so we can document this directly on that type instead of duplicating everything on a AccountIdV1 type.
That way, these comments pointing to the AccountId type still lead to the actual docs so I left them mostly as-is.

objects/src/accounts/account_id/id.rs Outdated Show resolved Hide resolved
objects/src/accounts/account_id/id.rs Outdated Show resolved Hide resolved
objects/src/accounts/account_id/id_prefix.rs Show resolved Hide resolved
objects/src/accounts/account_id/id_v0.rs Outdated Show resolved Hide resolved
Base automatically changed from pgackst-account-id-error to next January 13, 2025 07:43
@PhilippGackstatter PhilippGackstatter merged commit cf53dfd into next Jan 13, 2025
9 checks passed
@PhilippGackstatter PhilippGackstatter deleted the pgackst-account-id-version-wrapper branch January 13, 2025 09:34
@PhilippGackstatter PhilippGackstatter mentioned this pull request Jan 13, 2025
22 tasks
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