feat: allow custom client ids #362
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #362 +/- ##
=======================================
Coverage 99.85% 99.86%
=======================================
Files 14 14
Lines 706 730 +24
=======================================
+ Hits 705 729 +24
Misses 1 1 ☔ View full report in Codecov by Sentry. |
| // The following test cases are based on the test cases of ibc-go: | ||
| // https://github.com/cosmos/ibc-go/blob/e443a88e0f2c84c131c5a1de47945a5733ff9c91/modules/core/24-host/validate_test.go#L57 | ||
| ValidatePortIdentifierTestCase[] memory testCases = new ValidatePortIdentifierTestCase[](12); | ||
| ValidatePortIdentifierTestCase[] memory testCases = new ValidatePortIdentifierTestCase[](14); |
There was a problem hiding this comment.
Since we also pass the app address to this function now, we should add a test case for that.
There was a problem hiding this comment.
We have always passed the app address into this. Probably should have a test case
| function _addIBCApp(string memory portId, address app) private { | ||
| ICS26RouterStorage storage $ = _getICS26RouterStorage(); | ||
| require(address($._apps[portId]) == address(0), IBCPortAlreadyExists(portId)); | ||
| require(IBCIdentifiers.validateCustomIdentifier(bytes(portId)), IBCInvalidPortIdentifier(portId)); |
There was a problem hiding this comment.
I think the previous name was better, since we still use this for non-custom port identifiers.
There was a problem hiding this comment.
Maybe just validateIBCIdentifier? Or maybe we just have separate functions. Not sure if these has to always be the same (even if they are right now)
| onlyRole(CLIENT_ID_CUSTOMIZER_ROLE) | ||
| returns (string memory) | ||
| { | ||
| require(bytes(clientId).length != 0, IBCInvalidClientId(clientId)); |
There was a problem hiding this comment.
Should‘t we also check that the custom name is not in use? Because we'll just overwrite it here otherwise.
Description
closes: #360
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.