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

Fix UpdateMember encoding & decoding #32

Merged
merged 11 commits into from
Jan 30, 2023
Merged

Fix UpdateMember encoding & decoding #32

merged 11 commits into from
Jan 30, 2023

Conversation

NunoAlexandre
Copy link
Contributor

When testing the latest contract revision in add-withdaw-deposit, UpdateMember failed with a parsing error.
Here we fix all of these issues and improve the tests to verify the correctness of those changes. On the cent-chain side we tweak the encoding of the UpdateMember.validUntil to little endian so that it is parsed correctly on this side of the system.

NunoAlexandre
NunoAlexandre commented Jan 30, 2023
View reviewed changes
Copy link
Contributor Author

@NunoAlexandre NunoAlexandre left a comment

Choose a reason for hiding this comment

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

Some info for the reviewers

src/Connector.sol
@@ -141,7 +141,7 @@ contract CentrifugeConnector {
uint64 poolId,
bytes16 trancheId,
address user,
uint256 validUntil
uint64 validUntil
Copy link
Contributor Author

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 named amount, probably from a copy-paste from transfer I guess

Copy link
Contributor

Choose a reason for hiding this comment

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

Good find!

src/Messages.sol
* 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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Transfer and the storage items storing address now. I prefer to do that in a follow up PR unless if you think we shouldn't do this now?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's totally fine in a separate PR!

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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, poolId would be 1 + 8 = 9, and then we would be reading trancheId from byte at index 9, which would be another example of double-reading.

The docs were wrong indeed tho, as it's 9-24. Thanks for the catch!

src/Messages.sol
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) {
return abi.encodePacked(uint8(Call.UpdateMember), poolId, trancheId, user, bytes(hex"000000000000000000000000"), validUntil);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the decoding parses the first 20 bytes of user and skips the following 12, here we need to append 12 padding zeros to make it right.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@offerijns done 👍

test/Messages.t.sol
ConnectorMessages.formatUpdateMember(5, toBytes16(fromHex("010000000000000003")), 0x225ef95fa90f4F7938A5b34234d14768cB4263dd, 1657870537),
fromHex("04000000000000000500000000000000000000000000000009225ef95fa90f4f7938a5b34234d14768cb4263dd0000000000000000000000000000000000000000000000000000000062d118c9")
);
ConnectorMessages.formatUpdateMember(2, bytes16(hex"811acd5b3f17c06841c7e41e9e04cb1b"), 0x1231231231231231231231231231231231231231, 1706260138),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An interesting and frustration finding was that toBytes16(fromHex("811acd5b3f17c06841c7e41e9e04cb1b")) was retuning me an empty bytes16 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

Odd... does fromHex() expect a prepended 0x maybe?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

hieronx
hieronx approved these changes Jan 30, 2023
View reviewed changes
Copy link
Contributor

@hieronx hieronx left a comment

Choose a reason for hiding this comment

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

Looks good to me!

src/Messages.sol
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) {
return abi.encodePacked(uint8(Call.UpdateMember), poolId, trancheId, user, bytes(hex"000000000000000000000000"), validUntil);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment for this?

@NunoAlexandre NunoAlexandre merged commit e2832fe into add-withdaw-deposit Jan 30, 2023
@NunoAlexandre NunoAlexandre deleted the fix/add-member branch January 30, 2023 15:05
NunoAlexandre added a commit that referenced this pull request Feb 9, 2023
@ilinzweilin @AStox
@hieronx @NunoAlexandre
Connectors v1 (#16)
2af2d5c 
* add withdraw & deposit

* make withdraw function public

* add sendMessages to router

* update deploy script and add test

* switch Nomad to XCM router in tests

* add happy case deposit & withdraw tests

* remove unused code

* add xAppConnectionManager

* add assertions for withdraw happu case

* add edges cases for transfer route

* add edge case tests for transferTo

* remove misleading messages

* remove misleading messages

* update origin address

* Use correct checksummed origin address

* Strip out nomad and testing dependencies from XCMRouter. Add ping function for testing.

* Update router

* fix naming

* Update centrifuge origin address

* fix updateMember parsing

* roll back updateMember change

* Clean up

* forge install: memview-sol

* Fix all the tests

* Fix deps

* Remove yarn steps from workflows

* Remove deploy test

* Remove nomad router's send message logic and tests pertaining to it

* Fix memview-sol branch submodule not found

* Drop "== true" blocks

* Fix `UpdateMember` encoding & decoding (#32)

* Fix 9-24: trancheId (16 bytes) docs

* feat: Add `price` field to `AddTranche` (#33)

* Make `UpdateTokenPrice.price` and `Tranche.latestPrice` `u128` (#34)

* Use cent-chain AddTranche message

Use a cent-chain generated message to test AddTranche encoding and
decoding functions.

* Align tests with cent chain (#36)

* Use cent-chain generated UpdateMember for decoding

* Drop toBytes16

* Chore/add tests and optimizations (#38)

* wip: add transfer test

* move memberlist check from connectors to restricted token

* fix tests

* revert restricted.sol changes

* revert unwanted changes to erc20 and memberlist

* fix test

* Feat/domain update (#35)

* remove domainLookup

* require destination domain to be cent chain

* fix tests

* clean up

* fix up

* rename Domains to Domain

* Add new domain lookup

* update domain enum

* move formatDomain to messages.sol

* fix tests

* update enum and domain encoding

* update domain encoding to output bytes9

* Update src/Connector.sol

* update domain encoding and add more tests

---------

Co-authored-by: Jeroen Offerijns <[email protected]>

* Fine-tune transfers encoding, decoding, and tests (#39)

* Fix transfer encoding / decoding

We need to encode address as 32 bytes and take in consideration when
decoding. Also, amount should be encoded as a u128.

* Fix transfer decoding

* Cover transfer to centrifuge

* Fix docs - thanks @AStox

* Add TODO and more tests

* Update test/Connector.t.sol

---------

Co-authored-by: Adam Stox <[email protected]>
Co-authored-by: Jeroen Offerijns <[email protected]>
Co-authored-by: nuno <[email protected]>
Co-authored-by: Adam Stox <[email protected]>
Co-authored-by: Jeroen Offerijns <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hieronx hieronx hieronx approved these changes

@AStox AStox Awaiting requested review from AStox

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@NunoAlexandre @hieronx @AStox