solana: Add check for no registered transceivers #298
Conversation
johnsaigle
force-pushed
the
solana/empty-transceiver-guard
branch
2 times, most recently
from
59c79af to
518dceb
Compare
| // ensure that, if all transceivers have been disabled, that an error is returned. Otherwise it | ||
| // may be possible to permanently lose funds. | ||
| // No transceivers have been registered yet | ||
| if accs.common.config.next_transceiver_id == 0 { |
There was a problem hiding this comment.
Can this check be put in the common account context, maybe in the NotPausedConfig:
#[derive(Accounts)]
pub struct NotPausedConfig<'info> {
#[account(
constraint = !config.paused @ crate::error::NTTError::Paused,
constraint = config.next_transceiver_id != 0 @ NTTError::NoRegisteredTransceivers
)]
config: Account<'info, Config>,
}This would avoid having to put this check in each transfer instruction.
There was a problem hiding this comment.
If it is undesirable to do this check for everything that uses NotPausedConfig btw, adding a constraint in the common transfer context works, too:
#[account(
constraint = config.next_transceiver_id != 0 @ NTTError::NoRegisteredTransceivers
)]
pub config: NotPausedConfig<'info>,There was a problem hiding this comment.
Thanks for the review!
Yeah my intention is just to guard the Transfer functions so adding that constraint in the transfer context makes sense. I'll modify the code.
johnsaigle
force-pushed
the
solana/empty-transceiver-guard
branch
from
518dceb to
1ba37bd
Compare
|
EDIT: I made a mistake and it's fixed in the latest commit. I think this should fix the issues with the tests @a5-pickle I changed the code to add an account constraint instead of the ad-hoc check. Now that it exists, the unit tests is failing. I'm not sure whether this is revealing that we have a real bug in the transfer code or if this is instead an issue with mocking and that the problem exists in the way tests are set-up. @kcsongor when you have time I'd appreciate if you could maybe checkout the branch and take a look at the tests. I tried poking around the test suite for a while but didn't make much progress. While a transceiver appears to be registered during the SDK's |
johnsaigle
force-pushed
the
solana/empty-transceiver-guard
branch
from
1ba37bd to
a1ebc8b
Compare
| @@ -22,6 +22,10 @@ pub struct Redeem<'info> { | |||
| pub payer: Signer<'info>, | |||
|
|
|||
| // NOTE: this works when the contract is paused | |||
| #[account( | |||
| constraint = config.next_transceiver_id != 0 @ NTTError::NoRegisteredTransceivers, | |||
| constraint = config.enabled_transceivers.is_empty() @ NTTError::NoRegisteredTransceivers, | |||
There was a problem hiding this comment.
redeem() will fail if there aren't enough votes/attestations on a message and it's not possible to have attestations without transceivers, so we might as well fail early and with a more accurate error message.
johnsaigle
force-pushed
the
solana/empty-transceiver-guard
branch
from
c6d0161 to
7e19197
Compare
johnsaigle
force-pushed
the
solana/empty-transceiver-guard
branch
from
7e19197 to
42dddf7
Compare
| @@ -101,7 +110,8 @@ describe('example-native-token-transfers', () => { | |||
| amount, | |||
| recipientChain: 'ethereum', | |||
| recipientAddress: Array.from(user.publicKey.toBuffer()), // TODO: dummy | |||
| shouldQueue: false | |||
| shouldQueue: false, | |||
| config: await ntt.getConfig() | |||
There was a problem hiding this comment.
is this necessary? the function internally will make this exact function call anyway
| @@ -180,6 +190,7 @@ describe('example-native-token-transfers', () => { | |||
| const released = await ntt.redeem({ | |||
| payer, | |||
| vaa: vaaBuf, | |||
| config: await ntt.getConfig() | |||
|
@kcsongor thanks for the review. You're right that the arguments are redundant and don't seem to affect the test results. I've removed them in the latest commit. |
johnsaigle
dismissed
kcsongor’s stale review
Stale; changes have been addressed
johnsaigle
force-pushed
the
solana/empty-transceiver-guard
branch
from
9918acd to
f0cf7e2
Compare
johnsaigle
force-pushed
the
solana/empty-transceiver-guard
branch
from
f0cf7e2 to
aaf1e43
Compare
johnsaigle
force-pushed
the
solana/empty-transceiver-guard
branch
from
aaf1e43 to
2e6c3be
Compare
Add guard on transfer instructions to prevent a transfer from being processed when no transceivers are registered. This is done by checking the `next_transceiver_id` field in the Config. This field auto-increments when a transceiver is added. If this value is 0, then the program is in a state where no transceivers have been registered.
johnsaigle
force-pushed
the
solana/empty-transceiver-guard
branch
from
2e6c3be to
86a3a59
Compare
|
Changes merged through #514 |
Add guard on transfer instructions to prevent a transfer from being processed when no transceivers are registered. This is done by checking the
next_transceiver_idfield in the Config. This field auto-increments when a transceiver is added. If this value is 0, then the program is in a state where no transceivers have been registered.See also #289 for equivalent checks on the EVM side.
Note that, at this point, there is no ability for Transceivers to be disabled within the Solana implementation. This reduces the risk explored in #289 as the only moment where no Transceivers are registered occurs just after deployment.