fix: enforce ERC-55 addresses #368
Conversation
chris13524
force-pushed
the
fix/enforce-erc55
branch
from
3311509 to
81b0259
Compare
src/model/types/account_id.rs
Outdated
| .get_or_init(|| regex::Regex::new(r"^eip155:\d+:0x(?<address>[0-9a-fA-F]{40})$").unwrap()); | ||
|
|
||
| if let Some(caps) = pattern.captures(account_id) { | ||
| let address = &caps["address"]; |
There was a problem hiding this comment.
What's the error handling behavior if address isn't present? This is like a coding failure here, not a runtime error. If the regex matches then address must exist which is why I did [] instead of .get()
There was a problem hiding this comment.
Index<&str> only panics if there isn't a capture group with the given name.
But this could panic if I made a mistake in the regex such as conditionally matching. Looks like there's a slightly better alternative, extract, which panics if the number of possible matching groups in this Captures value is not fixed to N in all circumstances. So although it doesn't support named capture groups, it would result in catching any introduction of | operators in the regex. I switched to this in 1658283
There was a problem hiding this comment.
What's the error handling behavior if
addressisn't present? This is like a coding failure here, not a runtime error. If the regex matches thenaddressmust exist which is why I did[]instead of.get()
Initially I'd just log the error and figure it out if/when it becomes a problem but you found a better approach now 👍
Description
Followup to #364. CAIP-10 requires the use of ERC-55, and until now we have been permissive about this and normalized to ERC-55 internally. However, we should follow the spec and require ERC-55 now so people don't get lazy in the future.
This PR updates the logic to require ERC-55 to construct an
AccountId. If this fails validation, an error will be returned to the appropriate party.Logs show that the only non-compliance with ERC-55 is in the use of the
/notifyendpoint from 1 obscure project with 0 subscribers, and 1 client that appeared today. I decided not to build backwards-compatibility for these 2 entities.Also I manually checked that all existing database rows are all eip155 accounts and are ERC-55 compliant.
How Has This Been Tested?
Existing tests
Due Diligence