Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: CCIP-2465 ccip-enumerableMap for addressToBytesMap #13890

Closed
wants to merge 20 commits into from
Closed

feat: CCIP-2465 ccip-enumerableMap for addressToBytesMap #13890

wants to merge 20 commits into from

Conversation

defistar
Copy link
Contributor

@defistar defistar commented Jul 18, 2024 •
edited
Loading

EnumerableMap for mapping Bytes32 to Bytes

JIRA Ticket

https://smartcontract-it.atlassian.net/browse/CCIP-2465

Background

  • CCIP has rateLimiter with tokenMapping from local to remote chain
  • The mappings are grouped by per remoteChainSelector to its localTokenAddress => remoteTokenAddress
  • at moment the mapping for tokenAddresses are using
mapping(uint64 remoteChainSelector => EnumerableMapAddresses.AddressToBytes32Map tokensLocalToRemote) internal
    s_rateLimitedTokensLocalToRemote;

this needs to be changed to use

mapping(uint64 remoteChainSelector => EnumerableMapAddresses.AddressToBytesMap tokensLocalToRemote) internal
    s_rateLimitedTokensLocalToRemote;

Change List

  1. new library EnumerableMapBytes32.sol which contains mapping from Bytes32 => Bytes
  2. use the new library in EnumerableMapAddresses.sol for mapping arrangement functions for Address => Bytes

@defistar defistar requested a review from RensR as a code owner July 18, 2024 14:46
@defistar defistar self-assigned this Jul 18, 2024
@defistar defistar added the ready for review PR is ready for code review label Jul 18, 2024
RensR
RensR reviewed Jul 18, 2024
View reviewed changes
contracts/src/v0.8/shared/enumerable/EnumerableMapAddresses.sol Outdated
@@ -2,17 +2,19 @@
pragma solidity ^0.8.0;

import {EnumerableMap} from "../../vendor/openzeppelin-solidity/v4.8.3/contracts/utils/structs/EnumerableMap.sol";
import "./CCIPEnumerableMap.sol";
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use explicit imports

RensR
RensR reviewed Jul 18, 2024
View reviewed changes
contracts/src/v0.8/shared/enumerable/CCIPEnumerableMap.sol Outdated
*
* - `key` must be in the map.
*/
// solhint-disable-next-line chainlink-solidity/prefix-internal-functions-with-underscore
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we can turn off this rule for the entire file instead

RensR
RensR reviewed Jul 18, 2024
View reviewed changes
contracts/src/v0.8/shared/enumerable/EnumerableMapAddresses.sol Outdated
// solhint-disable-next-line chainlink-solidity/prefix-internal-functions-with-underscore

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: whitespace

RensR
RensR reviewed Jul 18, 2024
View reviewed changes
contracts/src/v0.8/shared/enumerable/CCIPEnumerableMap.sol Outdated
* array of CCIPEnumerableMap.
* ====
*/
library CCIPEnumerableMap {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's remove the CCIP naming

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, something like EnumerableMapBytes32 to be consistent with EnumerableMapAddresses

elatoskinas
elatoskinas reviewed Jul 18, 2024
View reviewed changes
contracts/src/v0.8/shared/enumerable/CCIPEnumerableMap.sol Outdated
// solhint-disable-next-line chainlink-solidity/prefix-internal-functions-with-underscore
function remove(Bytes32ToBytesMap storage map, bytes32 key) internal returns (bool) {
delete map._values[key];
return map._keys.remove(key);
Copy link
Contributor

Choose a reason for hiding this comment

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

q: does this correctly remove all bytes values, since bytes is a dynamic array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a test for this. will test on varying bytes size

contracts/src/v0.8/shared/test/enumerable/EnumerableMapAddresses.t.sol Outdated
assertTrue(s_addressToBytesMap.contains(address(this)));
(bool success, bytes memory value) = s_addressToBytesMap.tryGet(address(this));
assertTrue(success);
assertTrue(keccak256(value) == keccak256(MOCK_BYTES_VALUE));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assertTrue(keccak256(value) == keccak256(MOCK_BYTES_VALUE));
assertEq(value, MOCK_BYTES_VALUE);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

contracts/src/v0.8/shared/test/enumerable/EnumerableMapAddresses.t.sol Outdated
assertTrue(!s_addressToBytesMap.contains(address(this)));
assertTrue(s_addressToBytesMap.set(address(this), MOCK_BYTES_VALUE));
assertTrue(s_addressToBytesMap.contains(address(this)));
assertTrue(keccak256(s_addressToBytesMap.get(address(this))) == keccak256(MOCK_BYTES_VALUE));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assertTrue(keccak256(s_addressToBytesMap.get(address(this))) == keccak256(MOCK_BYTES_VALUE));
assertEq(s_addressToBytesMap.get(address(this)), MOCK_BYTES_VALUE);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

contracts/src/v0.8/shared/test/enumerable/EnumerableMapAddresses.t.sol Outdated
assertTrue(!s_addressToBytesMap.contains(address(this)));
assertTrue(s_addressToBytesMap.set(address(this), MOCK_BYTES_VALUE));
assertTrue(s_addressToBytesMap.contains(address(this)));
assertTrue(
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: same here - you can use assertEq

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@defistar defistar requested review from RensR and elatoskinas July 19, 2024 04:09
@defistar defistar requested a review from elatoskinas July 19, 2024 10:17
@defistar defistar requested review from rstout and 0xsuryansh and removed request for rstout July 22, 2024 09:23
RayXpub
RayXpub reviewed Jul 31, 2024
View reviewed changes
contracts/src/v0.8/shared/enumerable/EnumerableMapAddresses.sol
pragma solidity ^0.8.0;

import {EnumerableMap} from "../../vendor/openzeppelin-solidity/v4.8.3/contracts/utils/structs/EnumerableMap.sol";
import {EnumerableMapBytes32} from "./EnumerableMapBytes32.sol";

// TODO: the lib can be replaced with OZ v5.1 post-upgrade, which has AddressToAddressMap and AddressToBytes32Map
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think this is no longer true as OZ v5 does not support AddressToBytesMap

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 31, 2024 •
edited
Loading

LCOV of commit 4603c09 during Solidity Foundry #40448

Summary coverage rate:
  lines......: 98.7% (1838 of 1862 lines)
  functions..: 96.6% (344 of 356 functions)
  branches...: 90.7% (782 of 862 branches)

Files changed coverage rate: n/a

@defistar defistar enabled auto-merge August 1, 2024 14:48
@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@defistar defistar mentioned this pull request Aug 2, 2024
Merged
@defistar defistar closed this Aug 11, 2024
auto-merge was automatically disabled August 11, 2024 20:15

Pull request was closed

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

@RayXpub RayXpub RayXpub left review comments

@makramkd makramkd makramkd approved these changes

@elatoskinas elatoskinas elatoskinas approved these changes

@RensR RensR Awaiting requested review from RensR

@0xsuryansh 0xsuryansh Awaiting requested review from 0xsuryansh

Assignees

@defistar defistar

Labels
ready for review PR is ready for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@defistar @RensR @makramkd @elatoskinas @RayXpub