Skip to content

Anchor upgrade#62

Open
crypt0miester wants to merge 8 commits into
blockworks-foundation:masterfrom
crypt0miester:anchor-upgrade
Open

Anchor upgrade#62
crypt0miester wants to merge 8 commits into
blockworks-foundation:masterfrom
crypt0miester:anchor-upgrade

Conversation

@crypt0miester

@crypt0miester crypt0miester commented Aug 3, 2024

Copy link
Copy Markdown

gm,

Hope this is what is needed.

main notes:

  • zero_copy(unsafe) is used so that the Voter could have the same old behavior.
  • lifetimes were added to make the build compile for close_voter() and registrar.max_vote_weight()
  • fixed readme to mention the latest rust version
  • with regards to the discriminator changing, reading the source code in spl-governance it checks for is_initialized() which includes the old discriminator. and the method for deserializing the VoterWeightRecord has this check. this dictates that it would work. I'm not entirely sure tho.

Regards,
miester

@mschneider

mschneider commented Aug 6, 2024

Copy link
Copy Markdown

Hi, I think it would be best if you fork this repo. We would happily add it to the README as a starting point for people who want to make their own deployment with the newest version. Hope you understand, we don't have time to review this as we are happy with our own deployment and wouldn't like to touch it unless absolutely necessary.
Screenshot 2024-08-06 at 3 19 18 PM

@crypt0miester

Copy link
Copy Markdown
Author

Hi, I think it would be best if you fork this repo. We would happily add it to the README as a starting point for people who want to make their own deployment with the newest version. Hope you understand, we don't have time to review this as we are happy with our own deployment and wouldn't like to touch it unless absolutely necessary.
Screenshot 2024-08-06 at 3 19 18 PM

noted sir.

as far as the attached image is concerned, it is generated by the cargo builds to remove any compilation issues.

rm Cargo.lock && anchor build

@SebastianBor

Copy link
Copy Markdown

Hi, I think it would be best if you fork this repo.

This change doesn't introduce any new functionality. It only upgrades the project to the latest Anchor version because it's no longer possible to build it. Both regular and verifiable Anchor builds don't work any longer.

If you still think this should not be merged please let us know and we would fork it

@mschneider

Copy link
Copy Markdown

just worried there is definitely change in there that I wouldn't be able to sign off in short time, specifically in the test suite, if you want it merged you would need to rework those parts. i triggered a CI run, so all of the formal checks are run at least once and you get an idea for what needs to change apart from that.

addin_program_id,
processor!(voter_stake_registry::entry),
// processor!(voter_stake_registry::entry),
None,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why would you not bind the entry point?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

new anchor complains due to mismatched types.
image

solana-program inherits the entry from the .so if the .so files exists.
I couldn't find another way to adding the builtin_function.

// &authority.pubkey(),
// &payer.pubkey(),
// &authority.pubkey(),
// ),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

how come this is no longer needed? in general we prefer not to keep unused code in comments

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

sorry I missed this, the add_signatory() has changed in spl-gov, newer tests did not have it when creating a proposal.

I readded it with the new version of the instruction.

Comment thread programs/voter-stake-registry/tests/program_test/governance.rs Outdated
assert_eq!(deposit.is_used, true);
assert_eq!(deposit.amount_deposited_native, 12000);
assert_eq!(deposit.amount_initially_locked_native, 12000);
let amount_deposited_native = deposit.amount_deposited_native;

@mschneider mschneider Aug 7, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

please keep the diff minimal when possible

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

we have to have this due to zero_copy(unsafe) otherwise pointers would give undefined behavior. we need to copy the field to a local variable.

image

@mschneider

mschneider commented Aug 7, 2024

Copy link
Copy Markdown

i also would appreciate a comment if this code is 100% backwards compatible and how that was verified. the zero copy implementation did change substantially in v27 afaik

@crypt0miester

Copy link
Copy Markdown
Author

just worried there is definitely change in there that I wouldn't be able to sign off in short time, specifically in the test suite, if you want it merged you would need to rework those parts. i triggered a CI run, so all of the formal checks are run at least once and you get an idea for what needs to change apart from that.

please note that soteria has been deprecated sec3 has moved to a newer version called sec3-pro. and cargo audit requires new IDs to be added in .cargo/audit.toml to pass as per solana-program-library

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