refactor: use requires to assert and automatically log richer information on failure#65
refactor: use requires to assert and automatically log richer information on failure#65snawaz wants to merge 3 commits intosnawaz/documentfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Dodecahedr0x
left a comment
Dodecahedr0x
left a comment
There was a problem hiding this comment.
require_n_accounts is more restrictive than the current implem. I don't thinks it's a bad thing (tx data is already a limit) but it's also unnecessary IMO in this PR
Plus small suggestions
| @@ -0,0 +1,240 @@ | |||
| /// | |||
There was a problem hiding this comment.
I feel like requires are part of the program more than the API
There was a problem hiding this comment.
api crate has code as well, that validates and returns error. we should use requires there as well, else we'll see only errors like InvalidAccountData in the logs without any context.
| #[macro_export] | ||
| macro_rules! require { | ||
| ($cond:expr, $error:expr) => {{ | ||
| if !$cond { |
There was a problem hiding this comment.
they can all use core::hiint::unlikely to improve perfs (tried it locally, up to 600 CU gains on some instructions, requires nightly)
There was a problem hiding this comment.
Yes. I could use that. But 600 CU is very unlikely (pun not intended), because if usually consumes 4 to 6 CU.
| ); | ||
|
|
||
| assert_owner!(rent_pda_info, &pinocchio_system::ID); | ||
| require!( |
There was a problem hiding this comment.
assert_owner uses the more efficient address_eq instead of the internal == of owned_by. Could introduce require_owner or use require_keys_eq(unsafe { x.owner() }, y)
There was a problem hiding this comment.
Makes sense. I think pinocchio developers overlooked it when implementing owned_by function.
| let [ | ||
| queue_info, // force multi-line | ||
| _system_program_info, | ||
| ] = require_n_accounts!(accounts, 2); |
| owner_info, // force multi-line | ||
| ephemeral_ata_info, | ||
| recipient_info, | ||
| ] = require_n_accounts!(accounts, 3); |
There was a problem hiding this comment.
I discussed these cases with @GabrielePicco and so the earlier form [a, b, c, ..] is made stricter and this is equivalent to [a, b, c] that ensures we do not unecessarily pass more accounts than used by the instruction, to keep the tx-size minimal.
| lamports_pda_info, | ||
| payer_info, | ||
| destination_info, | ||
| ] = require_n_accounts!(accounts, 4); |
There was a problem hiding this comment.
contradicts the len() >= 6 below
There was a problem hiding this comment.
Eagle eyes! 👁️
It seems we do not have any test for this ix because tests pass!
I used require_n_accounts_with_ignored(accounts, 4) now, that ignores additonal accounts.
3 participants
Question:
Are we passing more accounts than needed by processors?Based on the existing code, I had to use.require_n_accounts_with_ignored!()but I want to use the stricter formrequire_n_accounts!()