chore(solana): handle action items vol 4 #857
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #857 +/- ##
=======================================
Coverage 99.89% 99.89%
=======================================
Files 22 22
Lines 982 982
=======================================
Hits 981 981
Misses 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mariuszzak
force-pushed
the
mariuszzak/action_items_part_4
branch
from
3f8b62f to
3a500ef
Compare
| /// | ||
| /// Use this for instructions that can be called both directly by users and via CPI from trusted programs. | ||
| pub fn validate_direct_or_whitelisted_cpi( | ||
| pub fn require_direct_call_or_whitelisted_caller( |
| pub rent: Sysvar<'info, Rent>, | ||
|
|
||
| /// Required by BPF Loader Upgradeable's upgrade instruction | ||
| pub clock: Sysvar<'info, Clock>, |
There was a problem hiding this comment.
Should we remove the clock account requirement to be consistent with other implementations?
There was a problem hiding this comment.
Good catch!
The clock sysvar is required by BPF Loader Upgradeable's upgrade instruction. We cannot use Clock::get()? for cpi call here:
anchor_lang::solana_program::program::invoke_signed(
&upgrade_ix,
&[
ctx.accounts.program_data.to_account_info(),
ctx.accounts.program.to_account_info(),
ctx.accounts.buffer.to_account_info(),
ctx.accounts.spill.to_account_info(),
ctx.accounts.rent.to_account_info(),
ctx.accounts.clock.to_account_info(),
ctx.accounts.upgrade_authority.to_account_info(),
],
&[&[
AccessManager::UPGRADE_AUTHORITY_SEED,
target_program.as_ref(),
&[bump],
]],
)?;
| /// Instructions sysvar for CPI validation | ||
| /// CHECK: Address constraint verifies this is the instructions sysvar | ||
| #[account(address = anchor_lang::solana_program::sysvar::instructions::ID)] | ||
| pub instructions_sysvar: AccountInfo<'info>, |
There was a problem hiding this comment.
Is it ok if we remove the cpi validation?
There was a problem hiding this comment.
Yes, we agreed that we can drop cpi validation in initialize. It would need to be called just between the program deployment and the initialization so it would be very unlikely.
| pub mod utils; | ||
|
|
||
| // Re-export events for use by instructions | ||
| pub use events::{ |
There was a problem hiding this comment.
I think the usage is better if we don't reexport. That way you know immediately what the struct represents.
|
|
||
| // Re-export events for use by instructions | ||
| pub use events::{ | ||
| AccessManagerUpdated, AckPacketEvent, ClientAddedEvent, ClientUpdatedEvent, IBCAppAdded, |
There was a problem hiding this comment.
Some events have Event in the name and some doesn't. Should we unify that and add the Event to the existing ones? Like AccessManagerUpdated -> AccessManagerUpdatedEvent
There was a problem hiding this comment.
Good catch! I see that non-prefix version is dominant in our codebase (and consistent with eth implementation). I made such adjustements:
ics26-router
| Old Name | New Name |
|---------------------------|------------------------|
| SendPacketEvent | PacketSent |
| WriteAcknowledgementEvent | AcknowledgementWritten |
| AckPacketEvent | PacketAcknowledged |
| TimeoutPacketEvent | PacketTimedOut |
| ClientAddedEvent | ClientAdded |
| ClientUpdatedEvent | ClientUpdated |
| IBCAppAddedEvent | IBCAppAdded |
| NoopEvent | Noop |
| AccessManagerUpdatedEvent | AccessManagerUpdated |
access-manager
| Old Name | New Name |
|----------------------|-----------------|
| RoleGrantedEvent | RoleGranted |
| RoleRevokedEvent | RoleRevoked |
| ProgramUpgradedEvent | ProgramUpgraded |
| ProgramExtendedEvent | ProgramExtended |
Used past tense for events that would conflict with Accounts struct names (e.g., SendPacket accounts -> PacketSent event).
Description
This PR addresses the issues we discovered during the code walk-through session.
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.