-
Notifications
You must be signed in to change notification settings - Fork 50
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 LSP11 package #918
Conversation
packages/lsp11-contracts/contracts/ILSP11UniversalSocialRecovery.sol
Outdated
Show resolved
Hide resolved
packages/lsp11-contracts/contracts/LSP11UniversalSocialRecovery.sol
Outdated
Show resolved
Hide resolved
packages/lsp11-contracts/contracts/ILSP11UniversalSocialRecovery.sol
Outdated
Show resolved
Hide resolved
packages/lsp11-contracts/contracts/ILSP11UniversalSocialRecovery.sol
Outdated
Show resolved
Hide resolved
packages/lsp11-contracts/contracts/ILSP11UniversalSocialRecovery.sol
Outdated
Show resolved
Hide resolved
packages/lsp11-contracts/contracts/ILSP11UniversalSocialRecovery.sol
Outdated
Show resolved
Hide resolved
packages/lsp11-contracts/contracts/ILSP11UniversalSocialRecovery.sol
Outdated
Show resolved
Hide resolved
packages/lsp11-contracts/contracts/LSP11UniversalSocialRecovery.sol
Outdated
Show resolved
Hide resolved
packages/lsp11-contracts/contracts/LSP11UniversalSocialRecovery.sol
Outdated
Show resolved
Hide resolved
packages/lsp11-contracts/contracts/LSP11UniversalSocialRecovery.sol
Outdated
Show resolved
Hide resolved
packages/lsp11-contracts/contracts/LSP11UniversalSocialRecovery.sol
Outdated
Show resolved
Hide resolved
packages/lsp11-contracts/contracts/LSP11UniversalSocialRecovery.sol
Outdated
Show resolved
Hide resolved
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.
Added first round of review.
You also need to register the package in the release-please-config.json
file.
lsp-smart-contracts/release-please-config.json
Lines 176 to 202 in f4def52
}, | |
"packages/lsp17contractextension-contracts": { | |
"component": "lsp17contractextension-contracts", | |
"package-name": "@lukso/lsp17contractextension-contracts", | |
"include-component-in-tag": true, | |
"release-type": "node", | |
"bump-minor-pre-major": true, | |
"bump-patch-for-minor-pre-major": false, | |
"draft": false, | |
"prerelease-type": "rc", | |
"extra-files": [ | |
"packages/lsp17contractextension-contracts/contracts/Version.sol" | |
] | |
}, | |
"packages/lsp23-contracts": { | |
"component": "lsp23-contracts", | |
"package-name": "@lukso/lsp23-contracts", | |
"include-component-in-tag": true, | |
"release-type": "node", | |
"bump-minor-pre-major": true, | |
"bump-patch-for-minor-pre-major": false, | |
"draft": false, | |
"prerelease-type": "rc" | |
} | |
}, | |
"$schema": "https://raw.githubusercontent.com/googleapis/release-please/main/schemas/config.json" | |
} |
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.
added 2nd round of review for the Solidity code
|
||
// Errors | ||
// solhint-disable no-global-import | ||
import "./LSP11Errors.sol"; |
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.
Here since there are so many, the specific import syntax would be quite big, so maybe we could do:
import "./LSP11Errors.sol"; | |
import ./LSP11Errors.sol" as Errors; |
And then reference like that:
revert Errors.NotAGuardian(...)
wdyt?
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 see the value in adding it
using ECDSA for *; | ||
|
||
/// @dev The default recovery delay set to 40 minutes. | ||
uint256 private constant _DEFAULT_RECOVERY_DELAY = 40 minutes; |
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 it make sense to show that as public
for block explorers?
|
||
// if there is a secret require a commitment first | ||
if (_secretHash != bytes32(0)) { | ||
bytes32 saltedHash = keccak256(abi.encode(account, secretHash)); |
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.
encode or encode packed?
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.
No preference
What does this PR introduce?
🚀 Feature
PR Checklist
npm run lint
&&npm run lint:solidity
(solhint)npm run format
(prettier)npm run build
npm run test