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.

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

Chore/add tests and optimizations #38

Merged
merged 6 commits into from
Feb 6, 2023

Conversation

AStox
Copy link
Contributor

@AStox AStox commented Feb 3, 2023

No description provided.

@AStox AStox changed the base branch from main to add-withdaw-deposit February 3, 2023 03:20
@AStox AStox marked this pull request as ready for review February 3, 2023 14:42
src/Connector.sol Show resolved Hide resolved
src/Connector.sol Show resolved Hide resolved
src/Connector.sol Outdated Show resolved Hide resolved
@@ -176,6 +180,44 @@ contract ConnectorTest is Test {
bridgedConnector.updateTokenPrice(poolId, trancheId, price);
}

function testTransfer(uint64 poolId, string memory tokenName, string memory tokenSymbol, bytes16 trancheId, uint128 price, uint32 destinationDomain, address user, uint256 amount, uint64 validUntil) public {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👌

Copy link
Contributor

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

LGTM but would like to have @offerijns's look on this as well

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.

Just two questions

For the error naming, I think we should just try to move to Solidity 0.8 soon and then we can use typed errors.

@@ -72,7 +72,7 @@ contract ERC20 {
emit Transfer(src, dst, wad);
return true;
}
function mint(address usr, uint wad) external virtual auth {
function mint(address usr, uint wad) public virtual auth {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we changing 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.

Ah my bad. This was left over from the mint change.

@@ -28,7 +28,7 @@ contract Memberlist {
}

function updateMember(address usr, uint validUntil) public auth {
require((safeAdd(block.timestamp, minimumDelay)) < validUntil, "invalid-validUntil");
require((safeAdd(block.timestamp, minimumDelay)) < validUntil, "cent/invalid-validUntil");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeh, in fact a lot of other errors are prefixed with CentrifugeConnector/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just removed this change to keep these contracts unaltered

@AStox AStox requested a review from hieronx February 6, 2023 15:07
@AStox AStox merged commit 78edd87 into add-withdaw-deposit Feb 6, 2023
@AStox AStox deleted the chore/add-tests-and-optimizations branch February 6, 2023 15:18
NunoAlexandre added a commit that referenced this pull request Feb 9, 2023
* 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants