feat: read faucet asset-callbacks bit from bridge registry#3074
feat: read faucet asset-callbacks bit from bridge registry#3074partylikeits1983 wants to merge 2 commits into
Conversation
The bridge hardcoded the enable_callbacks bit when deriving faucet assets (push.0 in unlock_and_send, push.1 in write_mint_note_storage), which breaks as soon as a configured faucet's actual callbacks bit differs. Store the bit in the bridge's faucet registry at registration time (new asset_callbacks field on ConversionMetadata; registry value is now [1, is_native, asset_callbacks, 0]) and read it back at both call sites. Closes #2963
partylikeits1983
requested review from
Fumuran,
PhilippGackstatter and
mmagician
and removed request for
PhilippGackstatter
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
LGTM, but as usual, take my review of agglayer code with a grain of salt.
| #! | ||
| #! Also registers: | ||
| #! 3. faucet_registry_map: [0, 0, faucet_id_suffix, faucet_id_prefix] -> [1, is_native, 0, 0] | ||
| #! 3. faucet_registry_map: [0, 0, faucet_id_suffix, faucet_id_prefix] -> [1, is_native, asset_callbacks, 0] |
There was a problem hiding this comment.
Nit: I'd call this enable_callbacks for consistency with asset::create_fungible_key and similar procedures.
| # --- Step 3: Store [1, is_native, asset_callbacks, 0] in faucet_registry_map --- | ||
| # KEY = [0, 0, faucet_id_suffix, faucet_id_prefix], VALUE = [1, is_native, asset_callbacks, 0] | ||
| # The trailing 0 of VALUE is supplied by the stack's bottom pads. |
There was a problem hiding this comment.
Nit: To someone unfamiliar with the code, the 1 in [1, is_native, ...] should be named to give a hint what it means.
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Not a very detailed review - but looks good! Thank you! I left a couple of comments inline, the main one is about a potentially different approach.
|
|
||
| impl ConversionMetadata { | ||
| /// Serializes the metadata to the 18-felt layout consumed by `CONFIG_AGG_BRIDGE`. | ||
| /// Serializes the metadata to the 19-felt layout consumed by `CONFIG_AGG_BRIDGE`. |
There was a problem hiding this comment.
Theoretically, we could compress this to 16 felts: scale, origin network, is_native, and asset_callbacks could all be encoded into 1 felt. Not sure if it is worth it though.
There was a problem hiding this comment.
I would leave it out for a future optimization if this turns out to be a bottleneck
| loc_load.REG_ASSET_CALLBACKS_LOC | ||
| loc_load.REG_IS_NATIVE_LOC push.FAUCET_REGISTERED_FLAG | ||
| # => [1, is_native, pad(16)] | ||
| # => [1, is_native, asset_callbacks, pad(16)] | ||
|
|
||
| loc_load.REG_FAUCET_ID_PREFIX_LOC loc_load.REG_FAUCET_ID_SUFFIX_LOC push.0.0 | ||
| # => [0, 0, faucet_id_suffix, faucet_id_prefix, 1, is_native, pad(16)] | ||
| # => [0, 0, faucet_id_suffix, faucet_id_prefix, 1, is_native, asset_callbacks, pad(16)] |
There was a problem hiding this comment.
This approach works, but there is also an alternative: we could expose something like has_callbacks on the faucet account, and then here, make an FPI call to the faucet to get the value of this flag. The main benefit is that this should prevent potential misconfigurations, and the main drawback is that it probably would take more code to implement.
There was a problem hiding this comment.
I'd stick with the current approach, and if needed later, we can switch it up as non-breaking due to how the code is structured, i.e. swapping out the storage load in get_faucet_asset_callbacks for an FPI call
mmagician
left a comment
mmagician
left a comment
There was a problem hiding this comment.
LGTM but this should also be applied to unlock_and_send path, as described in https://github.com/0xMiden/protocol/security/advisories/GHSA-fmr2-p8hq-xhmj
| loc_load.REG_ASSET_CALLBACKS_LOC | ||
| loc_load.REG_IS_NATIVE_LOC push.FAUCET_REGISTERED_FLAG | ||
| # => [1, is_native, pad(16)] | ||
| # => [1, is_native, asset_callbacks, pad(16)] | ||
|
|
||
| loc_load.REG_FAUCET_ID_PREFIX_LOC loc_load.REG_FAUCET_ID_SUFFIX_LOC push.0.0 | ||
| # => [0, 0, faucet_id_suffix, faucet_id_prefix, 1, is_native, pad(16)] | ||
| # => [0, 0, faucet_id_suffix, faucet_id_prefix, 1, is_native, asset_callbacks, pad(16)] |
There was a problem hiding this comment.
I'd stick with the current approach, and if needed later, we can switch it up as non-breaking due to how the code is structured, i.e. swapping out the storage load in get_faucet_asset_callbacks for an FPI call
|
|
||
| impl ConversionMetadata { | ||
| /// Serializes the metadata to the 18-felt layout consumed by `CONFIG_AGG_BRIDGE`. | ||
| /// Serializes the metadata to the 19-felt layout consumed by `CONFIG_AGG_BRIDGE`. |
There was a problem hiding this comment.
I would leave it out for a future optimization if this turns out to be a bottleneck
4 participants
Closes #2963.
The bridge hardcoded the
enable_callbacksbit when deriving faucet assets (push.0inunlock_and_send,push.1inwrite_mint_note_storage). This breaks as soon as a configured faucet's actual callbacks bit differs.This PR stores the bit in the bridge's faucet registry at registration time (new
asset_callbacksfield onConversionMetadata, registry value is now[1, is_native, asset_callbacks, 0]) and reads it back at both call sites.