Skip to content

fix(standards): bound transfer policy dispatch when no policy is active - #3881

Open
onurinanc wants to merge 9 commits into
nextfrom
fix-transfer-dispatch-empty-root-expiration
Open

onurinanc wants to merge 9 commits into
nextfrom
fix-transfer-dispatch-empty-root-expiration

Conversation

@onurinanc

Copy link
Copy Markdown
Collaborator

Summary

  • Apply the expiration limit and the pause check in invoke_transfer_policy before reading the active policy root, so both cover the empty-root branch.
  • Document in TokenPolicyManager that transfers anchored before a reserved policy is activated are bounded too.
  • Add tests pinning the default expiration limit and ERR_PAUSABLE_IS_PAUSED on a reserved-only faucet for both callbacks.
  • Regenerate the note cost tables.

Closes #3877.

@PhilippGackstatter PhilippGackstatter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Comment on lines 642 to +644
proc invoke_transfer_policy(slot_id: StorageSlotId, asset: Asset, custom_data: felt)
exec.expiration::apply_default
# => [slot_id_suffix, slot_id_prefix, ASSET_ID, ASSET_VALUE, custom_data]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this line was introduced in #3748, but I'm not sure this was correct. I think the idea was to let each concrete policy decide this. E.g. the blocklist applies this default itself, so removing it here wouldn't change the outcome for such a tx, but I haven't checked it holistically.

cc @bobbinth

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not 100% sure myself. IIUC, invoking expiration::apply_default will set the expiration upper bound on all transfer policies. The individual transfer policies would be able to decrease it - but never to increase it.

If so, I think that this may be too limiting as some transfer policies may be OK with pretty long expiration time. Maybe @partylikeits1983 or @mmagician remember the motivation?

Overall, it seems like we may not want to be invoking expiration::apply_default in such a blanket way. This is not from this PR, but maybe a good opportunity to fix it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bobbinth @PhilippGackstatter I agree that individual policies can have different expiration requirements. In #3512, we deliberately left expiration to individual policies; the basic allowlist and blocklist apply the 20 block default themselves.

My concern is that removing expiration::apply_default from invoke_transfer_policy could let callers keep using old faucet state from before a policy change or faucet pause.

I suggest the following:

  • Keep this PR’s expiration and pause checks, including when no policy is active.
  • In a followup PR, let the faucet developer configure the maximum callback expiration limit at account creation, with 20 blocks as the default. This would replace expiration::apply_default here with a new expiration::apply_configured_limit helper
  • Let individual policies impose stricter limits, while retaining the developer-set faucet-wide limit as an unconditional maximum.

For transactions invoking the faucet’s transfer callbacks, that limit would define the maximum age, in blocks, of the reference block at transaction inclusion.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is that removing expiration::apply_default from invoke_transfer_policy could let callers keep using old faucet state from before a policy change or faucet pause.

This is quite the edge case, but a valid point nonetheless. I'm fine merging the PR as is but would be good to open a follow-up issue to discuss potential solutions. I would like to avoid a configurable limit, as it feels unnecessarily complicated, but also can't come up with anything simpler that doesn't apply a blanket expiration if no policy is configured, which will likely be either too short or too long for most use cases.

@partylikeits1983 partylikeits1983 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, expiration and pause checks now apply when the active policy root is empty.

I think all that needs to be done is resolve merge conflicts and update the cost table.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

H-04: Transfer policy dispatcher skips the expiration limit when the active policy root is empty

4 participants