fix(recurring): grant and revoke relayer as separate calls - #1761
fix(recurring): grant and revoke relayer as separate calls#1761LeoSlrRf wants to merge 1 commit into
Conversation
|
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. |
Greptile SummaryThe PR replaces atomic relayer replacement with independent role grants and revocations and consolidates privileged operations under AccessControl.
Confidence Score: 4/5The PR should not merge until the published ERC20RecurringPaymentProxy artifact is synchronized with the new administration and relayer API. Package consumers currently receive an ABI that omits the newly added relayer methods and advertises methods removed from the contract bytecode, causing transaction encoding failures or reverts. Files Needing Attention: packages/smart-contracts/src/contracts/ERC20RecurringPaymentProxy.sol and packages/smart-contracts/src/lib/artifacts/ERC20RecurringPaymentProxy/0.1.0.json Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Admin[DEFAULT_ADMIN_ROLE holder] -->|grantRelayer| New[New relayer]
Admin -->|revokeRelayer| Old[Old relayer]
New -->|trigger due cycle| Proxy[Recurring payment proxy]
Old -->|while role remains| Proxy
Proxy -->|relayer fee| Caller[msg.sender]
Reviews (1): Last reviewed commit: "fix(recurring): grant and revoke relayer..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function grantRelayer(address relayer) external onlyRole(DEFAULT_ADMIN_ROLE) { | ||
| if (relayer == address(0)) revert ERC20RecurringPaymentProxy__ZeroAddress(); | ||
| _grantRole(RELAYER_ROLE, relayer); | ||
| } | ||
|
|
||
| /** | ||
| * @notice Revokes `RELAYER_ROLE`. Reverts if `relayer` does not hold the role. | ||
| */ | ||
| function revokeRelayer(address relayer) external onlyRole(DEFAULT_ADMIN_ROLE) { | ||
| if (!hasRole(RELAYER_ROLE, relayer)) { | ||
| revert ERC20RecurringPaymentProxy__NotRelayer(); | ||
| } | ||
| _revokeRole(RELAYER_ROLE, relayer); | ||
| } |
There was a problem hiding this comment.
When clients connect through the published erc20RecurringPaymentProxyArtifact, its ABI lacks grantRelayer and revokeRelayer while retaining the removed setRelayer and Ownable methods, causing new operations to be unavailable and obsolete calls to revert against bytecode built from this source. Update the committed versioned artifact alongside this public API change.
Knowledge Base Used: Smart contracts and deployments
There was a problem hiding this comment.
New ABI is published in a PR up the graphite stack
1 participant
Remove
Ownabledependency; unify access control underAccessControlOwnablehas been removed fromERC20RecurringPaymentProxy. All privileged functions (pause,unpause,setFeeProxy,rescueTokens) now requireDEFAULT_ADMIN_ROLEinstead ofonlyOwner, and thetransferOwnershipcall in the constructor has been dropped.The single
setRelayerfunction (which atomically swapped one relayer for another) has been replaced with two explicit functions:grantRelayer(address)— grantsRELAYER_ROLEto an address; reverts on the zero address.revokeRelayer(address)— revokesRELAYER_ROLE; reverts withERC20RecurringPaymentProxy__NotRelayerif the address does not currently hold the role.Both functions are gated by
DEFAULT_ADMIN_ROLE. This makes it possible to have multiple concurrent relayers competing forrelayerFee, since_payRelayerpaysmsg.sender.A new custom error
ERC20RecurringPaymentProxy__NotRelayeris introduced to give a clear revert reason when attempting to revoke a role from an address that does not hold it.Tests have been updated accordingly: the
Ownershipsuite is replaced by anAdmin rolesuite coveringDEFAULT_ADMIN_ROLEgrant/revoke via the standardAccessControlinterface, and thesetRelayersuite is expanded intograntRelayer and revokeRelayerwith coverage for zero-address grants, invalid revokes, event emissions, and non-admin access attempts.