-
Notifications
You must be signed in to change notification settings - Fork 75
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: nikolay <[email protected]>
Test Results 23 files + 3 303 suites +43 55m 38s ⏱️ + 14m 1s 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]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
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.
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?
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
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.
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 |
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 shouldn't be in the layer zero dir in it's final form.
Move this to tools/whbar
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.
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, |
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.
Q: If _from
is not msg.sender then it presumes an approval exists for the contract right?
If not do we need a check?
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.
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.
// SPDX-License-Identifier: Apache-2.0 | ||
pragma solidity >=0.4.9 <0.9.0; | ||
|
||
library HederaResponseCodes { |
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.
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
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.
Done.
abstract contract HTSConnector is OFTCore, KeyHelper, ExpiryHelper, HederaTokenService { | ||
address public htsTokenAddress; | ||
|
||
event CreatedToken(address tokenAddress); |
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.
Isnt it more standard, and language rich, TokenCreated?
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.
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
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Quality Gate failedFailed conditions |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
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); |
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.
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, |
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.
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, |
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.
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); |
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.
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 () => { |
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.
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
Description:
LZ is deployed on Hedera and we'd like to dig into see what's possible when considering HTS and it's representation fo ERC20 and ERC 721s.
Explore the LZ offering, and gain familiarity as a team and POC so cross-chain bridging of HTS tokens
Self-improvement docs:
Tasks
Related issue(s):
Fixes #3339
Notes for reviewer:
Checklist