Skip to content

Conversation

bmuddha
Copy link
Collaborator

@bmuddha bmuddha commented Mar 27, 2025

core record type now supports versioning via enum representation sync instruction also supports versioning to reflect changes in Record type evolution.

  • some dependencies version adjustements

core record type now supports versioning via enum representation
sync instruction also supports versioning to reflect changes in
Record type evolution.

+ some dependencies version adjustements
@bmuddha bmuddha self-assigned this Mar 28, 2025
Copy link
Contributor

@GabrielePicco GabrielePicco left a comment

Choose a reason for hiding this comment

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

LGTM! The only consideration is that it may be worth adding a discriminator to the account, so that we can easily retrieve all the records by filtering for it. A follow-up consideration is that if we have a discriminator, the enum used for versioning could be avoided, as we could just bump the discriminator with the same effect.

Copy link
Contributor

@taco-paco taco-paco left a comment

Choose a reason for hiding this comment

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

Looks great! Just a small comment on .take usage in getters

@bmuddha
Copy link
Collaborator Author

bmuddha commented Mar 28, 2025

LGTM! The only consideration is that it may be worth adding a discriminator to the account, so that we can easily retrieve all the records by filtering for it. A follow-up consideration is that if we have a discriminator, the enum used for versioning could be avoided, as we could just bump the discriminator with the same effect.

How borsh will be able to differentiate versions, if we don't use enums, every version bump will keep breaking existing code...
And borsh already encodes discriminant for the enum, so adding one explicitly is kind of redundant in this case
@GabrielePicco

@bmuddha bmuddha requested a review from taco-paco March 28, 2025 08:19
Copy link

@thlorenz thlorenz left a comment

Choose a reason for hiding this comment

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

Just minor stuff, but maybe you can address

@GabrielePicco
Copy link
Contributor

GabrielePicco commented Mar 28, 2025

How borsh will be able to differentiate versions, if we don't use enums, every version bump will keep breaking existing code...
And borsh already encodes discriminant for the enum, so adding one explicitly is kind of redundant in this case

They will just be treated as different accounts, since the discriminator is different, each parsing it's own struct. New version won't break previous ones.

What I'm thinking is that most likely we need to retrieve all records with something like:

let filters = vec![RpcFilterType::Memcmp(Memcmp::new(
    0,
    MemcmpEncodedBytes::Bytes(vec![1, 0, 0, 0, 0, 0, 0, 0]),
))];

But I see your point, if Borsh serialize the enum in a predictable way, in the first bytes of the accounts, we could use it as a discriminator. It's gonna be not standard (typically the discriminator is the first 8 bytes) and I guess variable size, but if the above is true, it's fine with me

@GabrielePicco GabrielePicco self-requested a review March 28, 2025 13:01
@bmuddha bmuddha requested a review from thlorenz April 1, 2025 09:04
Copy link

@thlorenz thlorenz left a comment

Choose a reason for hiding this comment

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

Overall LGTM, but please make those error msgs a bit more helpful by including pubkeys :)

@bmuddha bmuddha merged commit eba7644 into master Apr 2, 2025
3 checks passed
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.

4 participants