-
Notifications
You must be signed in to change notification settings - Fork 6
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.
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
Fix UpdateMember
encoding & decoding
#32
Changes from all commits
7e465f8
30d5db7
3fd29fd
db2f5c3
4f55b21
4930ce2
3f80815
3c7be17
6c7829f
c4fb963
b8ab0c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,7 +50,7 @@ library ConnectorMessages { | |
function formatAddTranche(uint64 poolId, bytes16 trancheId, string memory tokenName, string memory tokenSymbol) internal pure returns (bytes memory) { | ||
// TODO(nuno): Now, we encode `tokenName` as a 128-bytearray by first encoding `tokenName` | ||
// to bytes32 and then we encode three empty bytes32's, which sum up to a total of 128 bytes. | ||
// Add support to actually encode `tokennName` fully as a 128 bytes string. | ||
// Add support to actually encode `tokenName` fully as a 128 bytes string. | ||
return abi.encodePacked(uint8(Call.AddTranche), poolId, trancheId, stringToBytes32(tokenName), bytes32(""), bytes32(""), bytes32(""), stringToBytes32(tokenSymbol)); | ||
} | ||
|
||
|
@@ -96,25 +96,27 @@ library ConnectorMessages { | |
* 0: call type (uint8 = 1 byte) | ||
* 1-8: poolId (uint64 = 8 bytes) | ||
* 9-25: trancheId (16 bytes) | ||
* 26-46: user (Ethereum address, 20 bytes) | ||
* 47-78: validUntil (uint256 = 32 bytes) | ||
* 25-45: user (Ethereum address, 20 bytes - Skip 12 bytes from 32-byte addresses) | ||
* 57-65: validUntil (uint64 = 8 bytes) | ||
* | ||
* TODO: use bytes32 for user (for non-EVM compatibility) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @offerijns I started working on this but we would need to also update There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's totally fine in a separate PR! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yah separating issues into separate PRs is a good idea. I'll note here so I don't forget though, it looks like we're double-reading a byte here. Tranche ID is 9-25, while user is 25-45. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @AStox I first thought this as well but it doesn't seem to be the case: poolId = uint64(_msg.indexUint(1, 8));
trancheId = bytes16(_msg.index(9, 16));
user = address(bytes20(_msg.index(25, 20)));
validUntil = uint64(_msg.indexUint(57, 8)); So, The docs were wrong indeed tho, as it's |
||
*/ | ||
function formatUpdateMember(uint64 poolId, bytes16 trancheId, address user, uint256 validUntil) internal pure returns (bytes memory) { | ||
return abi.encodePacked(uint8(Call.UpdateMember), poolId, trancheId, user, validUntil); | ||
function formatUpdateMember(uint64 poolId, bytes16 trancheId, address user, uint64 validUntil) internal pure returns (bytes memory) { | ||
// NOTE: Since parseUpdateMember parses the first 20 bytes of `user` and skips the following 12 | ||
// here we need to append 12 zeros to make it right. Drop once we support 32-byte addresses. | ||
return abi.encodePacked(uint8(Call.UpdateMember), poolId, trancheId, user, bytes(hex"000000000000000000000000"), validUntil); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the decoding parses the first 20 bytes of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @offerijns done 👍 |
||
} | ||
|
||
function isUpdateMember(bytes29 _msg) internal pure returns (bool) { | ||
return messageType(_msg) == Call.UpdateMember; | ||
} | ||
|
||
function parseUpdateMember(bytes29 _msg) internal pure returns (uint64 poolId, bytes16 trancheId, address user, uint256 validUntil) { | ||
|
||
function parseUpdateMember(bytes29 _msg) internal pure returns (uint64 poolId, bytes16 trancheId, address user, uint64 validUntil) { | ||
poolId = uint64(_msg.indexUint(1, 8)); | ||
trancheId = bytes16(_msg.index(9, 16)); | ||
user = address(bytes20(_msg.index(25, 20))); | ||
// TODO: skip 12 padded zeroes from address | ||
validUntil = uint256(_msg.index(45, 32)); | ||
validUntil = uint64(_msg.indexUint(57, 8)); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,41 +91,41 @@ contract MessagesTest is Test { | |
|
||
function testUpdateMemberEncoding() public { | ||
assertEq( | ||
ConnectorMessages.formatUpdateMember(5, toBytes16(fromHex("010000000000000003")), 0x225ef95fa90f4F7938A5b34234d14768cB4263dd, 1657870537), | ||
fromHex("04000000000000000500000000000000000000000000000009225ef95fa90f4f7938a5b34234d14768cb4263dd0000000000000000000000000000000000000000000000000000000062d118c9") | ||
); | ||
ConnectorMessages.formatUpdateMember(2, bytes16(hex"811acd5b3f17c06841c7e41e9e04cb1b"), 0x1231231231231231231231231231231231231231, 1706260138), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An interesting and frustration finding was that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Odd... does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't really look like it based on how we use it elsewhere actually. |
||
hex"040000000000000002811acd5b3f17c06841c7e41e9e04cb1b12312312312312312312312312312312312312310000000000000000000000000000000065b376aa" | ||
); | ||
} | ||
|
||
function testUpdateMemberDecoding() public { | ||
(uint64 decodedPoolId, bytes16 decodedTrancheId, address decodedUser, uint256 decodedValidUntil) = ConnectorMessages.parseUpdateMember(fromHex("04000000000000000500000000000000000000000000000009225ef95fa90f4f7938a5b34234d14768cb4263dd0000000000000000000000000000000000000000000000000000000062d118c9").ref(0)); | ||
assertEq(uint(decodedPoolId), uint(5)); | ||
assertEq(decodedTrancheId, toBytes16(fromHex("010000000000000003"))); | ||
assertEq(decodedUser, 0x225ef95fa90f4F7938A5b34234d14768cB4263dd); | ||
assertEq(decodedValidUntil, uint(1657870537)); | ||
(uint64 decodedPoolId, bytes16 decodedTrancheId, address decodedUser, uint64 decodedValidUntil) = ConnectorMessages.parseUpdateMember(fromHex("040000000000000002811acd5b3f17c06841c7e41e9e04cb1b12312312312312312312312312312312312312312312312312312312312312310000000065B376AA").ref(0)); | ||
assertEq(uint(decodedPoolId), uint(2)); | ||
assertEq(decodedTrancheId, hex"811acd5b3f17c06841c7e41e9e04cb1b"); | ||
assertEq(decodedUser, 0x1231231231231231231231231231231231231231); | ||
assertEq(decodedValidUntil, uint(1706260138)); | ||
} | ||
|
||
function testUpdateMemberEquivalence( | ||
uint64 poolId, | ||
bytes16 trancheId, | ||
address user, | ||
uint256 amount | ||
uint64 validUntil | ||
) public { | ||
bytes memory _message = ConnectorMessages.formatUpdateMember( | ||
poolId, | ||
trancheId, | ||
user, | ||
amount | ||
validUntil | ||
); | ||
( | ||
uint64 decodedPoolId, | ||
bytes16 decodedTrancheId, | ||
address decodedUser, | ||
uint256 decodedAmount | ||
uint64 decodedValidUntil | ||
) = ConnectorMessages.parseUpdateMember(_message.ref(0)); | ||
assertEq(uint256(decodedPoolId), uint256(poolId)); | ||
assertEq(uint(decodedPoolId), uint(poolId)); | ||
assertEq(decodedTrancheId, trancheId); | ||
assertEq(decodedUser, user); | ||
assertEq(decodedAmount, amount); | ||
assertEq(uint(decodedValidUntil), uint(validUntil)); | ||
} | ||
|
||
function testUpdateTokenPriceEncoding() public { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was wrong and often
validUntil
was namedamount
, probably from a copy-paste fromtransfer
I guessThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find!