-
Notifications
You must be signed in to change notification settings - Fork 223
copy in oft202 #1229
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
copy in oft202 #1229
Conversation
EDIT: resolved lint is failing due to other examples
|
Seems like I must update the lockfile, as if not, the following error appears:
|
The lint errors are erratic, now it's:
|
857ab0e
to
11511c8
Compare
7727dcf
to
c9e57c3
Compare
4bce532
to
f463bd8
Compare
New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is unstable ownership?A new collaborator has begun publishing package versions. Package stability and security risk may be elevated. Try to reduce the number of authors you depend on to reduce the risk to malicious actors gaining access to your supply chain. Packages should remove inactive collaborators with publishing rights from packages on npm. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
765d260
to
afbdde0
Compare
fec9846
to
696d063
Compare
Signed-off-by: Ryan Goulding <[email protected]>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
0df94d6
to
e81765e
Compare
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
|
||
import "@layerzerolabs/solidity-examples/contracts/token/oft/v2/OFTV2.sol"; | ||
|
||
contract MyEndpointV1OFTV2 is OFTV2 { |
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.
Worth adding some solidity natspec warnings here stating that this is an EndpointV1 contract in bold, I get that the name is EndpointV1OFTV2, but devs in the past wonder about the inheritance tree and the "solidity-examples" package doesn't do a good job of explaining 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.
*/ | ||
constructor(address _endpoint, address _owner) NonblockingLzApp(_endpoint) Ownable(_owner) {} | ||
constructor(address _endpoint) NonblockingLzApp(_endpoint) {} |
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.
Is the reason you removed ownable here because you are using OZ v4?
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.
Yes, I mentioned OZ v5 -> v4 downgrade in the PR body
@@ -44,7 +43,7 @@ contract MyOApp is OApp { | |||
* protocol-specific settings for the application, such as managing | |||
* message libraries and configuring messaging parameters. | |||
*/ | |||
constructor(address _endpoint, address _delegate) OApp(_endpoint, _delegate) Ownable(_delegate) {} | |||
constructor(address _endpoint, address _delegate) OApp(_endpoint, _delegate) {} |
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.
Same question here, OZ v4 I'm guessing?
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.
yes, refer to above comment's reply
config: { | ||
sendLibrary: '0x4f7cd4DA19ABB31b0eC98b9066B9e857B1bf9C0E', | ||
sendLibrary: '0x6862b19f6e42a810946B9C782E6ebE26Ad266C84', |
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.
Would be good to add some comments here similar to what we've done with the solana repo to callout that the send and receive libraries are SendUln301 and ReceiveUln301 on each chain.
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.
on this note actually, are these addresses in lz-definitions? always thought that having string literal values were quite cryptic. would be much better if we can have them like the endpoint IDs, referanceable via constants
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.
}, | ||
ulnConfig: { | ||
confirmations: BigInt(1), | ||
requiredDVNs: ['0x3a74f7174709842d3b8a14ce60b4aa2499f2a2f2'], | ||
requiredDVNs: ['0x8eebf8b423b73bfca51a1db4b7354aa0bfca9193'], // LayerZero Labs DVN |
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.
DVNs on the other hand are just whatever DVN is available per chain, since DVNs are only available as a V2 property, and thus, don't need to worry about mixing up with V1 Oracles.
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.
Do you mean, the label here is not as necessary ? Per my above comment, I think it's too cryptic to have only the string literal.
"compile:forge": "forge build", | ||
"compile:hardhat": "hardhat compile", | ||
"lint": "$npm_execpath run lint:js && $npm_execpath run lint:sol", | ||
"lint:fix": "eslint --fix '**/*.{js,ts,json}' && prettier --write . && solhint 'contracts/**/*.sol' --fix --noPrompt", | ||
"lint:js": "eslint '**/*.{js,ts,json}' && prettier --check .", | ||
"lint:sol": "solhint 'contracts/**/*.sol'", | ||
"test": "$npm_execpath run test:forge && $npm_execpath run test:hardhat", | ||
"test:anchor": "anchor test", |
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 think we ran into issues with keeping anchor test in the overarching test command as it would fail when it hit the CI due to keys not matching.
Was this an issue at all for you?
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 added this in here to ensure parity with oft-solana
, which as you are implying, does not have the anchor test included as part of the general test
command. I also didn't add it to general test
here.
@@ -48,8 +64,8 @@ | |||
"@layerzerolabs/toolbox-hardhat": "~0.6.5", | |||
"@nomicfoundation/hardhat-ethers": "^3.0.5", | |||
"@nomiclabs/hardhat-ethers": "^2.2.3", | |||
"@openzeppelin/contracts": "^5.0.2", | |||
"@openzeppelin/contracts-upgradeable": "^5.0.2", | |||
"@openzeppelin/contracts": "^4.6.0", |
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.
We should probably call out in the README somewhere that this only works for OZ V4.
|
||
if (transactions.length === 0) { | ||
logger.info('The LzApp is wired, no action is necessary') |
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.
Is there a reason you removed the logger.info calls?
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.
485bdba
to
bb31210
Compare
Signed-off-by: Ryan Goulding <[email protected]> Co-authored-by: Ryan Goulding <[email protected]> Co-authored-by: layerzero-bot <[email protected]> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Matthew Krak <[email protected]>
This PR amends the lzapp-migration example in the following ways:
wire.ts
to support Solana tooRemaining todos:
config.get.ts
workbuildAdapterParams
from evm v1 sdkNotes
@openzeppelin/contracts": "^5.0.2
and@openzeppelin/contracts-upgradeable": "^5.0.2
to@openzeppelin/contracts": "^4.6.0
and@openzeppelin/contracts-upgradeable@^4.6.0
Ownable
that causes the import of@layerzerolabs/solidity-examples/contracts/token/oft/v2/OFTV2.sol
to break compilation since it used v4 and did not provide an argument toOwner
.lzApp
contract should also be reverted to be the same as the original and not provide the constructor argument toOwner
to accurately replicate v1 deployments that used v4.