Conversation
aganglada
force-pushed
the
feat/compliance-controller
branch
from
abafb3c to
eb49115
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@aganglada Could also run |
| // Fall back to per-address compliance status map | ||
| const status = this.state.walletComplianceStatusMap[address]; | ||
| return status?.blocked ?? false; | ||
| } |
There was a problem hiding this comment.
Case-sensitive address lookup may miss blocked wallets
High Severity
isWalletBlocked uses case-sensitive Array.includes() for the blocklist and case-sensitive object key lookup for the status map. Ethereum addresses are hex and case-insensitive, so a casing mismatch between the blocklist and the queried address (e.g., 0xABC vs 0xabc) would cause a sanctioned wallet to be incorrectly reported as not blocked. The PhishingController in this same monorepo already uses toLowerCase() for address comparisons to avoid this problem.
cryptodev-2s
left a comment
cryptodev-2s
left a comment
There was a problem hiding this comment.
I took an initial look and left a few suggestions to help reduce the boilerplate.
| for (let idx = 0; idx < statuses.length; idx++) { | ||
| const callerAddress = addresses[idx]; | ||
| draftState.walletComplianceStatusMap[callerAddress] = statuses[idx]; | ||
| } |
There was a problem hiding this comment.
Batch compliance mapping assumes API preserves input ordering
High Severity
checkWalletsCompliance maps results to state using positional indexing — addresses[idx] is used as the key for statuses[idx]. This assumes the batch API returns results in exactly the same order as the input addresses array. If the API returns results in a different order (or a different count), the wrong blocked status gets persisted against the wrong address. For an OFAC compliance controller, this could mark a sanctioned wallet as safe or a clean wallet as blocked. A safer approach would be to key by statuses[idx].address (which comes from result.address in the API response) rather than by positional index from the caller's input array.
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
Hello there! I had some comments relating to aligning with best practices.
| * | ||
| * @returns The blocked wallets information. | ||
| */ | ||
| async fetchBlockedWallets(): Promise<BlockedWalletsInfo> { |
There was a problem hiding this comment.
Nit: Perhaps this should be called updateBlockedWallets? Besides fetching it's also updating the state.
| }: { | ||
| messenger: ComplianceServiceMessenger; | ||
| fetch: typeof fetch; | ||
| complianceApiUrl: string; |
There was a problem hiding this comment.
Is there a reason why you need this class to take a URL? This allows this class to theoretically represent any endpoint that satisfies the API being called here, however, it means that the clients need to store what that URL is, so that knowledge is not kept here.
If we are using multiple URLs for different environments what are your thoughts on having this class take an environment or env option and then constructing the URL here?
| * is missing or stale. Call once after construction to ensure the blocklist | ||
| * is ready for `isWalletBlocked` lookups. | ||
| */ | ||
| async initialize(): Promise<void> { |
There was a problem hiding this comment.
This is not an official standard, however, in other controllers such as AccountTreeController use init rather than initialize for the name of the "post-construction initialization" method. What are your thoughts on renaming this method?
Alternatively, what are your thoughts on isWalletBlocked making an API call the first time it's called?
There was a problem hiding this comment.
Agree with init.
Regarding isWalletBlocked making the api call, this will mean the first call with be blocking and we don't want that
| * @returns `true` if the wallet is blocked according to cached data, | ||
| * `false` otherwise. | ||
| */ | ||
| isWalletBlocked(address: string): boolean { |
There was a problem hiding this comment.
If you truly don't want to make an API call here, since this method only accesses state, what are your thoughts on extracting this to a selector? See here for documentation on how to do this: https://github.com/MetaMask/core/blob/main/docs/code-guidelines/controller-guidelines.md#expose-derived-state-using-selectors-instead-of-getters (See here for defining selectors with arguments.)
| ComplianceServiceCheckWalletComplianceAction, | ||
| ComplianceServiceCheckWalletsComplianceAction, | ||
| ComplianceServiceFetchBlockedWalletsAction, | ||
| ComplianceServiceMethodActions, |
There was a problem hiding this comment.
Typically we don't export this type, because we don't need it outside the package. Thoughts on removing?
| ComplianceServiceMethodActions, |
| ComplianceControllerFetchBlockedWalletsAction, | ||
| ComplianceControllerInitializeAction, | ||
| ComplianceControllerIsWalletBlockedAction, | ||
| ComplianceControllerMethodActions, |
There was a problem hiding this comment.
Same as above.
| ComplianceControllerMethodActions, |
aganglada
force-pushed
the
feat/compliance-controller
branch
from
b63359f to
06e890c
Compare
| if (blockedWallets?.addresses.includes(address)) { | ||
| return true; | ||
| } | ||
| return statusMap[address]?.blocked ?? false; |
There was a problem hiding this comment.
Case-sensitive address matching bypasses OFAC compliance check
High Severity
selectIsWalletBlocked uses case-sensitive comparison (addresses.includes(address)) and case-sensitive map key lookup (statusMap[address]). Ethereum addresses are case-insensitive — EIP-55 only adds checksum casing for display. A blocked address stored as 0xABC... won't match a lookup for 0xabc..., allowing a sanctioned wallet to bypass the compliance check. The PhishingController in this same monorepo normalizes addresses with toLowerCase() before comparison for exactly this reason. Both the selector and the state storage in checkWalletCompliance/checkWalletsCompliance need address normalization.
Additional Locations (1)
| ComplianceController, | ||
| getDefaultComplianceControllerState, | ||
| } from './ComplianceController'; | ||
| export { selectIsWalletBlocked } from './selectors'; |
There was a problem hiding this comment.
What do you think about modifying selectors.ts to place selectIsWalletBlocked (and any future selectors) in a complianceControllerSelectors object and exporting that instead?
| export { selectIsWalletBlocked } from './selectors'; | |
| export { complianceControllerSelectors } from './selectors'; |
My thought with the guidance here is that if a React component or hook is using multiple selectors from multiple packages, if each package's selectors are bundled together in one object, it reduces the chance that there are conflicts.
There was a problem hiding this comment.
This guidance prevents tree-shaking. Our team will have to think about this some more. I don't want this to block the PR, so we can come back to this later.
3 participants
Explanation
This PR adds a new
@metamask/compliance-controllerpackage to the monorepo. It provides OFAC compliance checking for wallet addresses by interfacing with the Compliance API (va-mmcx-compliance-api).The package consists of two main components:
ComplianceService(data service)Handles all HTTP communication with the Compliance API. It wraps three endpoints:
checkWalletCompliance(address)—GET /v1/wallet/:addressfor single address checkscheckWalletsCompliance(addresses)—POST /v1/wallet/batchfor batch address checksfetchBlockedWallets()—GET /v1/blocked-walletsfor the full OFAC blocklist with source metadataAll requests are wrapped with
createServicePolicyfor automatic retries, circuit breaking, and degraded state detection. Response validation uses@metamask/superstructschemas.ComplianceController(controller)Manages compliance state and provides a proactive caching pattern (similar to
PhishingController):initialize(), it fetches and caches the full blocked wallets list if it is missing or stale (configurable refresh interval, defaults to 1 hour)isWalletBlocked(address)performs a synchronous lookup against the cached blocklist — no API call neededcheckWalletCompliance,checkWalletsCompliance) remain available for fresh per-address checks when needed (e.g., before high-value transactions)clearComplianceState()resets all cached dataTest coverage
withControllerpatternnockfor HTTP stubbing with retry, circuit breaker, and degraded state scenariosReferences
va-mmcx-compliance-api(NestJS service providing OFAC SDN list checks)PhishingController(proactive blocklist caching),ClaimsController(PascalCase file structure), andSampleGasPricesController/SampleGasPricesService(messenger + service policy patterns)Checklist
Note
Medium Risk
Introduces a new network-facing compliance component that can affect address blocking decisions and relies on external API responses/caching semantics; however it is largely additive and well-tested without changing existing controller behavior.
Overview
Adds a new
@metamask/compliance-controllerpackage that supports OFAC compliance checks against the Compliance API, including aComplianceService(HTTP client with retry/circuit-break/degraded detection and response validation) and aComplianceControllerthat caches a blocked-wallets list plus per-address check results and exposes messenger actions (init,checkWalletCompliance,checkWalletsCompliance,updateBlockedWallets,clearComplianceState) andselectIsWalletBlocked.Wires the new package into repo metadata (README package list/dependency graph,
CODEOWNERS,teams.json,yarn.lock) and includes full unit test coverage plus standard package scaffolding (tsconfigs, typedoc, changelog, license).Written by Cursor Bugbot for commit e493973. This will update automatically on new commits. Configure here.