Conversation
There was a problem hiding this comment.
The transfer hook events have the same properties but different name to isolate them from the non-transfer hook events
bangyro
force-pushed
the
feat/transfer_hook_support2
branch
from
6dc418a to
41d18ae
Compare
|
/bot review |
|
Bot Agent progress:
Progress:
|
| require!( | ||
| transfer_hook_program.executable | ||
| && transfer_hook_program.key().ne(&crate::ID) | ||
| && transfer_hook_program | ||
| .key() | ||
| .ne(&ctx.accounts.token_program.key()), | ||
| PoolError::InvalidTransferHookProgram | ||
| ); |
There was a problem hiding this comment.
not necessary, just to be safe. ensuring that the transfer_hook_program is not one of the program involved in the transfer chain (dbc & token_2022)
| it("Initialize extra account meta list for transfer hook", async () => { | ||
| await initializeExtraAccountMetaList( | ||
| svm, | ||
| operator, | ||
| virtualPoolState.baseMint | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Need creator to init extra account meta list . Might cause issues if they do not init it
There was a problem hiding this comment.
I did not add support for transfer hook in the deprecated ix (ix_claim_protocol_fee and ix_zap_protocol_fee)
![metoeraag-bot[bot]](https://avatars.githubusercontent.com/u/126859799?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
Comment
Summary
Posted 4 validated inline finding(s) after final filtering.
Findings
- [P2]
programs/dynamic-bonding-curve/src/instructions/initialize_pool/ix_initialize_virtual_pool_with_token2022_transfer_hook.rs:106This account is only checked for!= Pubkey::default(), but it is not required to sign or be tied to the creator/partner. Because the new DAMM v2 migration path now requires the transfer-hook program id and authority to be revoked first, a typo or arbitrary third-party pubkey here can create a pool that nobody can ever migrate. Please require the hook authority to sign (or otherwise constrain it to a known signer-controlled authority) at initialization time. - [P2]
tests/create_pool_with_token2022_with_transfer_hook.tests.ts:214getMintwas changed to useunpackMint, which now validates the token program owner and returns the higher-levelMintshape. This call is reading a Token-2022 mint with the defaultTOKEN_PROGRAM_ID, so it will throw, and the latermintAuthorityOptionassertion no longer matches the returned type. Please passTOKEN_2022_PROGRAM_IDhere and checkmintAuthority === null/freezeAuthority === nullinstead of the old*AuthorityOptionfields. - [P2]
tests/first_swap_with_transfer_hook.tests.ts:326The helper is hard-codingreferralTokenAccount: program.programIdfor a test that is exercising the no-referral path. On-chain this optional account will be treated as present, which either fails account deserialization immediately (it's not a token account) or, even if it somehow got through, defeatsvalidate_contain_initialize_pool_ix_and_no_cpi()because that branch explicitly requiresreferral_token_account.is_none(). Please omit this optional account / passnullhere. - [P2]
tests/utils/token.ts:199After a transfer hook is revoked, the extension still exists andgetTransferHook(mintState)returns a struct whoseprogramIdisPublicKey.default()(the new migration tests assert exactly that). This helper only checks for a missing extension, so revoked mints are still treated as active-hook mints and extra accounts keep getting built for them. Downstream helpers likewithdrawLeftoverWithTransferHookthen pass hook accounts even though the on-chain transfer path now expects none. Please also short-circuit whentransferHook.programId.equals(PublicKey.default()).
Repo checks
Repo Checks
-
LLM checks planner: added package install step before running JS commands.
-
cargo fetch --locked: ok
-
bun install --frozen-lockfile: ok
bun install v1.3.2 (b131639c)
+ @anchor-lang/borsh@1.0.0
+ @solana/spl-token@0.4.13
+ @solana/spl-token-metadata@0.1.6
+ @solana/web3.js@1.98.4
+ @types/bn.js@5.1.6
+ @types/chai@4.3.20
+ @types/mocha@9.1.1
+ chai@4.5.0
+ decimal.js@10.5.0
+ litesvm@0.1.0
+ mocha@9.2.2
+ prettier@2.8.8
+ ts-mocha@10.1.0
+ tsx@4.20.3
+ typescript@5.9.3
+ @anchor-lang/core@1.0.0
+ @metaplex-foundation/mpl-token-metadata@3.4.0
207 packages installed [25.78s]
- cargo check --workspace: ok
= note: this warning originates in the attribute macro `event_cpi` (in Nightly builds, run with -Z macro-backtrace for more info)
warning: use of deprecated function `dynamic_bonding_curve::claim_protocol_fee`: Use claim_protocol_fee2 through protocol_fee program instead
--> programs/dynamic-bonding-curve/src/lib.rs:60:12
|
60 | pub fn claim_protocol_fee<'info>(
| ^^^^^^^^^^^^^^^^^^
warning: use of deprecated function `dynamic_bonding_curve::zap_protocol_fee`: Use claim_protocol_fee2 through protocol_fee program instead
--> programs/dynamic-bonding-curve/src/lib.rs:80:12
|
80 | pub fn zap_protocol_fee<'info>(
| ^^^^^^^^^^^^^^^^
warning: use of deprecated struct `instructions::migration::dynamic_amm_v2::migration_damm_v2_create_metadata::MigrationDammV2CreateMetadataCtx`
--> programs/dynamic-bonding-curve/src/lib.rs:306:29
|
306 | ctx: Context<'info, MigrationDammV2CreateMetadataCtx<'info>>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
warning: use of deprecated function `dynamic_bonding_curve::migration_damm_v2_create_metadata`: It's unneeded. Will be removed in next release version
--> programs/dynamic-bonding-curve/src/lib.rs:305:12
|
305 | pub fn migration_damm_v2_create_metadata<'info>(
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
warning: use of deprecated field `instructions::migration::dynamic_amm_v2::migrate_damm_v2_initialize_pool::MigrateDammV2Ctx::migration_metadata`
--> programs/dynamic-bonding-curve/src/instructions/migration/dynamic_amm_v2/migrate_damm_v2_initialize_pool.rs:48:9
|
48 | pub migration_metadata: UncheckedAccount<'info>,
| ^^^^^^^^^^^^^^^^^^
warning: `dynamic-bonding-curve` (lib) generated 27 warnings (2 duplicates)
Checking dynamic-bonding-curve-sdk v0.1.0 (/data/workdir/2962945/MeteoraAg/dynamic-bonding-curve/dynamic-bonding-curve-sdk)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 25.83s
- bun run lint: failed
Checking formatting...
$ prettier */*.js "*/**/*{.js,.ts}" --check
[warn] tests/backwards_compatibility/utils.ts
[warn] tests/claim_protocol_liquidity_migration_fee.tests.ts
[warn] tests/instructions/creatorInstructions.ts
[warn] tests/instructions/dammV2Migration.ts
[warn] tests/migrate_damm_v2_dynamic_config.tests.ts
[warn] tests/utils/idl/damm_v2.ts
[warn] tests/utils/idl/dynamic_vault.ts
[warn] tests/utils/idl/locker.ts
[warn] tests/zap_protocol_fee.tests.ts
[warn] Code style issues found in 9 files. Forgot to run Prettier?
error: script "lint" exited with code 1
| pub transfer_hook_program: UncheckedAccount<'info>, | ||
|
|
||
| /// CHECK: transfer hook authority for the base mint | ||
| pub transfer_hook_authority: UncheckedAccount<'info>, |
There was a problem hiding this comment.
P2 This account is only checked for != Pubkey::default(), but it is not required to sign or be tied to the creator/partner. Because the new DAMM v2 migration path now requires the transfer-hook program id and authority to be revoked first, a typo or arbitrary third-party pubkey here can create a pool that nobody can ever migrate. Please require the hook authority to sign (or otherwise constrain it to a known signer-controlled authority) at initialization time.
There was a problem hiding this comment.
this is intentional
There was a problem hiding this comment.
allow for creator to have flexibility of setting the authority during dbc stage
There was a problem hiding this comment.
Intentional flexibility noted, but the core issue still stands: a mistyped or uncontrolled hook authority can make the hook impossible to revoke later and leave the pool unmigratable. If arbitrary-authority support is required, please at least add an explicit guard or warning for this irreversible footgun.
- address bot comments
|
/bot review |
|
Bot Agent progress:
Progress:
|
There was a problem hiding this comment.
Verdict
Comment
Summary
Posted 2 validated inline finding(s) after final filtering.
Findings
- [P2]
programs/dynamic-bonding-curve/src/instructions/initialize_pool/ix_initialize_virtual_pool_with_token2022_transfer_hook.rs:142This path creates the pool without rejecting a Token-2022 quote mint that itself has an active transfer hook. The new plumbing only carriesTransferHookBase/TransferHookBaseReferral, and every quote-side transfer in the diff still passesNonefor transfer-hook accounts (swap, protocol-fee claims, trading-fee claims, surplus/migration-fee withdrawals, etc.). If configs can point at a transfer-hook-enabled quote mint, pool creation will succeed but normal operations will fail at runtime withMissingRemainingAccountForTransferHook. Please either reject quote mints with active hooks here (or at config creation time), or add quote-side hook account support everywhere quote transfers happen. - [P2]
programs/dynamic-bonding-curve/src/instructions/swap/ix_swap.rs:333For transfer hook pools, onlyEvtSwap2WithTransferHookis emitted, but the legacyEvtSwapevent is no longer emitted at all. Non-transfer-hook pools still emit bothEvtSwapandEvtSwap2. This means downstream consumers that listen forEvtSwapon transfer hook pools will silently stop receiving events. Consider also emitting an equivalent legacy event or documenting this breaking change clearly.
Potential concerns (lower confidence)
- [P1]
programs/dynamic-bonding-curve/src/event/mod.rs:191Two events were removed:EvtPartnerWithdrawMigrationFeeandEvtClaimProtocolLiquidityMigrationFee. If any downstream indexers, event parsers, or off-chain systems depend on these event types, this is a breaking change. The changelog doesn't mention these removals. Were these events unused, or is this intentional? If intentional, please document it in the changelog.
Repo checks
Repo Checks
-
LLM checks planner: added package install step before running JS commands.
-
cargo fetch --locked: ok
-
bun install --frozen-lockfile: ok
bun install v1.3.2 (b131639c)
+ @anchor-lang/borsh@1.0.0
+ @solana/spl-token@0.4.13
+ @solana/spl-token-metadata@0.1.6
+ @solana/web3.js@1.98.4
+ @types/bn.js@5.1.6
+ @types/chai@4.3.20
+ @types/mocha@9.1.1
+ chai@4.5.0
+ decimal.js@10.5.0
+ litesvm@0.1.0
+ mocha@9.2.2
+ prettier@2.8.8
+ ts-mocha@10.1.0
+ tsx@4.20.3
+ typescript@5.9.3
+ @anchor-lang/core@1.0.0
+ @metaplex-foundation/mpl-token-metadata@3.4.0
207 packages installed [12.09s]
- cargo check --workspace: ok
= note: this warning originates in the attribute macro `event_cpi` (in Nightly builds, run with -Z macro-backtrace for more info)
warning: use of deprecated function `dynamic_bonding_curve::claim_protocol_fee`: Use claim_protocol_fee2 through protocol_fee program instead
--> programs/dynamic-bonding-curve/src/lib.rs:60:12
|
60 | pub fn claim_protocol_fee<'info>(
| ^^^^^^^^^^^^^^^^^^
warning: use of deprecated function `dynamic_bonding_curve::zap_protocol_fee`: Use claim_protocol_fee2 through protocol_fee program instead
--> programs/dynamic-bonding-curve/src/lib.rs:80:12
|
80 | pub fn zap_protocol_fee<'info>(
| ^^^^^^^^^^^^^^^^
warning: use of deprecated struct `instructions::migration::dynamic_amm_v2::migration_damm_v2_create_metadata::MigrationDammV2CreateMetadataCtx`
--> programs/dynamic-bonding-curve/src/lib.rs:306:29
|
306 | ctx: Context<'info, MigrationDammV2CreateMetadataCtx<'info>>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
warning: use of deprecated function `dynamic_bonding_curve::migration_damm_v2_create_metadata`: It's unneeded. Will be removed in next release version
--> programs/dynamic-bonding-curve/src/lib.rs:305:12
|
305 | pub fn migration_damm_v2_create_metadata<'info>(
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
warning: use of deprecated field `instructions::migration::dynamic_amm_v2::migrate_damm_v2_initialize_pool::MigrateDammV2Ctx::migration_metadata`
--> programs/dynamic-bonding-curve/src/instructions/migration/dynamic_amm_v2/migrate_damm_v2_initialize_pool.rs:48:9
|
48 | pub migration_metadata: UncheckedAccount<'info>,
| ^^^^^^^^^^^^^^^^^^
warning: `dynamic-bonding-curve` (lib) generated 27 warnings (2 duplicates)
Checking dynamic-bonding-curve-sdk v0.1.0 (/data/workdir/2962945/MeteoraAg/dynamic-bonding-curve/dynamic-bonding-curve-sdk)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 23.55s
- bun run lint: failed
Checking formatting...
$ prettier */*.js "*/**/*{.js,.ts}" --check
[warn] tests/backwards_compatibility/utils.ts
[warn] tests/claim_protocol_liquidity_migration_fee.tests.ts
[warn] tests/instructions/creatorInstructions.ts
[warn] tests/instructions/dammV2Migration.ts
[warn] tests/migrate_damm_v2_dynamic_config.tests.ts
[warn] tests/utils/idl/damm_v2.ts
[warn] tests/utils/idl/dynamic_vault.ts
[warn] tests/utils/idl/locker.ts
[warn] tests/zap_protocol_fee.tests.ts
[warn] Code style issues found in 9 files. Forgot to run Prettier?
error: script "lint" exited with code 1
| PoolError::InvalidTransferHookProgram | ||
| ); | ||
|
|
||
| let activation_point = process_initialize_virtual_pool_with_token2022( |
There was a problem hiding this comment.
P2 This path creates the pool without rejecting a Token-2022 quote mint that itself has an active transfer hook. The new plumbing only carries TransferHookBase / TransferHookBaseReferral, and every quote-side transfer in the diff still passes None for transfer-hook accounts (swap, protocol-fee claims, trading-fee claims, surplus/migration-fee withdrawals, etc.). If configs can point at a transfer-hook-enabled quote mint, pool creation will succeed but normal operations will fail at runtime with MissingRemainingAccountForTransferHook. Please either reject quote mints with active hooks here (or at config creation time), or add quote-side hook account support everywhere quote transfers happen.
There was a problem hiding this comment.
At config creation we reject quote mint with transfer hook with is_supported_quote_mint
There was a problem hiding this comment.
Thanks for the clarification — since config creation already rejects quote mints with active transfer hooks via is_supported_quote_mint, this instruction inherits that invariant and the concern here doesn’t apply.
| migration_threshold: config.migration_quote_threshold, | ||
| current_timestamp, | ||
| }); | ||
| if pool.is_transfer_hook_pool()? { |
There was a problem hiding this comment.
P2 For transfer hook pools, only EvtSwap2WithTransferHook is emitted, but the legacy EvtSwap event is no longer emitted at all. Non-transfer-hook pools still emit both EvtSwap and EvtSwap2. This means downstream consumers that listen for EvtSwap on transfer hook pools will silently stop receiving events. Consider also emitting an equivalent legacy event or documenting this breaking change clearly.
There was a problem hiding this comment.
EvtSwap is considered legacy. So we omit it for the transfer hook path
There was a problem hiding this comment.
Understood that EvtSwap is intentionally legacy here. The remaining concern is compatibility: transfer-hook pools will stop emitting it while other pools still do, so please at least document this as a breaking change for downstream indexers/consumers.
| emit_cpi!(EvtSwap { | ||
| pool: ctx.accounts.pool.key(), | ||
| config: ctx.accounts.config.key(), | ||
| trade_direction: trade_direction.into(), | ||
| has_referral, | ||
| params: swap_in_parameters, | ||
| swap_result, | ||
| amount_in: swap_result_2.included_fee_input_amount, | ||
| current_timestamp, | ||
| }); |
There was a problem hiding this comment.
I think this Evt is considered legacy, so I didn't add a WithTransferHook variant
| #[deprecated( | ||
| since = "0.1.7", | ||
| note = "It's unneeded. Will be removed in next release version" | ||
| )] | ||
| pub fn migration_damm_v2_create_metadata<'info>( | ||
| ctx: Context<'info, MigrationDammV2CreateMetadataCtx<'info>>, | ||
| ) -> Result<()> { | ||
| instructions::handle_migration_damm_v2_create_metadata(ctx) | ||
| } |
There was a problem hiding this comment.
should we remove this endpoint? I'll have to change the backwards compatibility test
1 participant
No description provided.