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

Improve frozen abi dx a bit and document it #2131

Merged
merged 6 commits into from
Jul 16, 2024

Conversation

ryoqun
Copy link
Member

@ryoqun ryoqun commented Jul 15, 2024

Problem

Frozen abi needs some love periodically.

Summary of Changes

It's another time again. This pr is cumulative one containing multiple changes relating to frozen abi.

See individual commits.

@ryoqun ryoqun requested review from apfitzge and yihau July 15, 2024 13:46
@ryoqun ryoqun changed the title Run frozen-abi tests earlier in ci Improve frozen abi devx a bit Jul 15, 2024
@ryoqun ryoqun marked this pull request as ready for review July 15, 2024 13:47
@apfitzge
Copy link

I assume this is a response to my discord comment here: https://discord.com/channels/428295358100013066/439194979856809985/1261428595814039603

While these changes fix it for my specific case of changing the hash function, I am still curious about the larger question of why the Bank needs a frozen Abi at all.
I understand why some other structs in accounts/runtime do. But Bank is the one that is most annoying, given its current state as a "god" class.

@ryoqun ryoqun changed the title Improve frozen abi devx a bit Improve frozen abi dx a bit Jul 15, 2024
@ryoqun ryoqun changed the title Improve frozen abi dx a bit Improve frozen abi dx a bit and document it Jul 15, 2024
@ryoqun
Copy link
Member Author

ryoqun commented Jul 15, 2024

While these changes fix it for my specific case of changing the hash function, I am still curious about the larger question of why the Bank needs a frozen Abi at all.

hehe, thanks for jumping in. i was too early to mark this pr out of draft.. I pushed a commit for the explanation. is there specific questions not answered by this still?: 99cdd64

@apfitzge
Copy link

While these changes fix it for my specific case of changing the hash function, I am still curious about the larger question of why the Bank needs a frozen Abi at all.

hehe, thanks for jumping in. i was too early to mark this pr out of draft.. I pushed a commit for the explanation. is there specific questions not answered by this still?: 99cdd64

Had responded on discord since you commented there.

But this explanation, to me, seems to justify that its' necessary because of the test structure...which we could also change to make that not required.

@ryoqun
Copy link
Member Author

ryoqun commented Jul 15, 2024

But this explanation, to me, seems to justify that its' necessary because of the test structure...which we could also change to make that not required.

yeah. as written in the commit, i was reluctant to copy all the type arguments of serialize_bank_snapshot_with. A good news is that i found a good compromise: d9615f4

@@ -431,6 +431,28 @@ impl<
}
}

#[cfg(not(target_os = "solana"))]
impl<T: Clone + std::cmp::Eq + std::hash::Hash + AbiExample, S: Clone + AbiExample> AbiExample

Choose a reason for hiding this comment

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

given the other changes, think we can drop adding this.

Copy link
Member Author

Choose a reason for hiding this comment

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

done by dropping the commit from the pr.

@ryoqun ryoqun force-pushed the frozen-abi-updates branch 2 times, most recently from 367d6c6 to da0afa1 Compare July 16, 2024 00:55
@ryoqun ryoqun requested a review from apfitzge July 16, 2024 01:22
@yihau
Copy link
Member

yihau commented Jul 16, 2024

looks good to me but I would like to wait for @apfitzge to have a chance to take a look!

@ryoqun ryoqun merged commit adf2447 into anza-xyz:master Jul 16, 2024
54 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.

3 participants