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

remove redundant schema def of epoch #944

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HaoranYi
Copy link

@HaoranYi HaoranYi commented Apr 20, 2024

Problem

Epoch def is added to BorshSchema twice.

Summary of Changes

Remove the redundant Epoch def addition to BorshSchema.

Fixes #

@HaoranYi HaoranYi force-pushed the serde/stake_delegation branch from d8f5919 to 3982d2c Compare April 22, 2024 15:38
@HaoranYi HaoranYi marked this pull request as ready for review April 22, 2024 17:20
@HaoranYi HaoranYi requested a review from joncinque April 22, 2024 17:20
@brooksprumo
Copy link

I'm not familiar with this code, so I defer to jon.

@brooksprumo brooksprumo removed their request for review April 22, 2024 17:34
Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

This code is the pasted output of the borsh 0.10 macro, because we can't easily derive the same trait twice without some crate in the middle, meaning we can't do:

use borsh0_10::BorshSchema as BorshSchema0_10;
use borsh1::BorshSchema as BorshSchema1;
...
#[derive(BorshSchema0_10, BorshSchema1)]

Your change is definitely correct, and you could likely even remove both lines because Epoch is just a u64 anyway! With that said, I'd prefer to not change it, since we're trying to give exactly the same code as the macro would output.

But going further... since the other borsh 0.10 bindings were deprecated in 1.18, we could even go as far as to remove all of its usage in 2.0 👀

@HaoranYi
Copy link
Author

Yeah. fix in 2.0 sounds good to me.

Copy link

mergify bot commented Dec 30, 2024

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

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.

3 participants