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

feat: add layer zero examples #3326

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

feat: add layer zero examples #3326

wants to merge 12 commits into from

Conversation

natanasow
Copy link
Collaborator

@natanasow natanasow commented Dec 11, 2024

@natanasow natanasow self-assigned this Dec 11, 2024
Copy link

github-actions bot commented Dec 11, 2024

Test Results

 23 files  + 3  303 suites  +43   55m 38s ⏱️ + 14m 1s
617 tests + 5  601 ✅  -  5  4 💤 ±0  12 ❌ +10 
817 runs  +60  801 ✅ +52  4 💤  - 2  12 ❌ +10 

For more details on these failures, see this check.

Results for commit 4cac520. ± Comparison against base commit b5e747f.

♻️ This comment has been updated with latest results.

Signed-off-by: nikolay <[email protected]>
@natanasow natanasow added this to the 0.64.0 milestone Dec 12, 2024
@natanasow natanasow added the dev tools Features enabling dev tool integration label Dec 12, 2024
@natanasow natanasow marked this pull request as ready for review December 16, 2024 15:02
Copy link
Member

@quiet-node quiet-node left a comment

Choose a reason for hiding this comment

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

Without context it's quite hard to follow the PR. Could you provide a design document or any documentation that outlines the purpose of the project? The document could also define terms such as OFT and OApp. I believe the document you shared with the team last Friday was very effective. Would you consider updating and including it in this project?

@quiet-node quiet-node added the internal For changes that affect the project's internal workings but not its outward-facing functionality. label Dec 16, 2024
Signed-off-by: nikolay <[email protected]>
@natanasow natanasow requested review from a team as code owners December 18, 2024 13:00
@natanasow natanasow requested a review from dalvizu December 18, 2024 13:00
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

Nice progress, some suggestions

npx hardhat test --grep "HTSAdapterTests @hedera @test" --network hedera_testnet
npx hardhat test --grep "HTSAdapterTests @bsc @test" --network bsc_testnet

### WHBAR flow
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shouldn't be in the layer zero dir in it's final form.
Move this to tools/whbar

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. I put it there because the WHBAR example is on another branch and if I create a duplication of it in that PR, there will be future conflicts to resolve. Will move it as you suggested after I finalize the README.

* @return amountReceivedLD The amount received in local decimals on the remote.
*/
function _debit(
address _from,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: If _from is not msg.sender then it presumes an approval exists for the contract right?
If not do we need a check?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was a check here and approval that had been executed afterward but after some feedback from Fernando, I decided to completely remove the approval from this call. It's user's/dapp's responsibility to ensure the token's approval beforehand like it's done in ERC20 swaps via 2 transactions so we don't need any extra actions here.

tools/layer-zero-example/contracts/WHBAR.sol Show resolved Hide resolved
// SPDX-License-Identifier: Apache-2.0
pragma solidity >=0.4.9 <0.9.0;

library HederaResponseCodes {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: might not be necessary to dupe the whole file.
Why not just pull in the response code you need and add a comment ref to the HederaResponseCodes in the smart contracts repo.
Once we get the npm package this will be easier

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

tools/layer-zero-example/hardhat.config.js Show resolved Hide resolved
tools/layer-zero-example/README.md Show resolved Hide resolved
abstract contract HTSConnector is OFTCore, KeyHelper, ExpiryHelper, HederaTokenService {
address public htsTokenAddress;

event CreatedToken(address tokenAddress);

Choose a reason for hiding this comment

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

Isnt it more standard, and language rich, TokenCreated?

Choose a reason for hiding this comment

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

As people could have already an HTS Token, or want to create one

  • is it an adapter escenario when it already exists?
  • Instead creating it inside, if you create outside and the you connect, it is almost The same as the previous sentence.

Whats de difference? Both scenarios could use the same code, we have at the connector

Does it makes sense to create the token inside in this connector?
If it can work in Adapter and connector mode, the only reason to use a connector is to avoid liquidity logic?

I think this doubts needs to be cased somehow on our doc, comparing adapter vs connector, with an existing token

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
3.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Copy link

codecov bot commented Dec 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.95%. Comparing base (b5e747f) to head (4cac520).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3326      +/-   ##
==========================================
- Coverage   84.91%   83.95%   -0.97%     
==========================================
  Files          69       69              
  Lines        4655     4688      +33     
  Branches     1042     1050       +8     
==========================================
- Hits         3953     3936      -17     
- Misses        397      424      +27     
- Partials      305      328      +23     
Flag Coverage Δ
config-service 98.14% <ø> (ø)
relay 79.72% <ø> (+0.96%) ⬆️
server 83.28% <ø> (ø)
ws-server 36.66% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 13 files with indirect coverage changes

Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

Nice work.
Some suggestions to make it easy to illustrate the E2E flow

* @dev In the case of OFT, address(this) and erc20 are the same contract.
*/
function token() public view returns (address) {
return address(this);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be htsTokenAddress instead of address(this)?
This contract doesn't conform to any token standard on it's own

IHederaTokenService.TokenKey[] memory keys = new IHederaTokenService.TokenKey[](2);
keys[0] = getSingleKey(
KeyType.ADMIN,
KeyType.PAUSE,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need the pause key. Let's keep this part simple with just admin and supply.

) payable OFTCore(8, _lzEndpoint, _delegate) {
IHederaTokenService.TokenKey[] memory keys = new IHederaTokenService.TokenKey[](2);
keys[0] = getSingleKey(
KeyType.ADMIN,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering if we even need the admin key or only the supply.
I forget if you have to specify admin in this case or if we can make the token immutable.
Even if we set it shouldn't it be a CONTRACT_ID type?

const contract = await contractFactory.deploy();
await contract.deployTransaction.wait();

console.log(`(${hre.network.name}) WHBAR to: ` + contract.address);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you log the tx hashes in these tasks.
Would be nice to follow the transactions in an E2E run

const amount = '100';

describe('HTSAdapterTests', function() {
it('@hedera @fund-and-approve transfer to adapter', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to other comment can you log all the state changing transaction hashes for all the tests to make it easy to tell the E2E story on a given run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev tools Features enabling dev tool integration internal For changes that affect the project's internal workings but not its outward-facing functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LZ <-> Hedera examples
4 participants