-
Notifications
You must be signed in to change notification settings - Fork 16
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
refactor!: upgrade @lukso/lsp-smart-contracts
to v0.14.0
#247
Conversation
@CallumGrindle @richtera @Hugoo So that it also encode the LSP3 / LSP4 metadata of Universal Profile / Digital Assets as a tools-lsp-factory/src/lib/services/digital-asset.service.ts Lines 435 to 436 in f8e0c40
|
Thanks for clear PR description |
@Hugoo this is ready for review |
@@ -120,6 +120,11 @@ async function deployLSP7DigitalAsset( | |||
) { | |||
const controllerAddress = await signer.getAddress(); | |||
|
|||
const lsp4TokenType = | |||
typeof digitalAssetDeploymentOptions.tokenType === 'string' | |||
? LSP4_TOKEN_TYPES[digitalAssetDeploymentOptions.tokenType] |
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.
Are we sure the value of
digitalAssetDeploymentOptions.tokenType
Will always be included in that array? Can thus lead to errors? Is the value of that variable verified?
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.
const { name, symbol, isNFT } = digitalAssetDeploymentOptions; | ||
const { name, symbol, tokenType, isNFT } = digitalAssetDeploymentOptions; | ||
|
||
const lsp4TokenType = typeof tokenType === 'string' ? LSP4_TOKEN_TYPES[tokenType] : tokenType; |
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 here I think it is safer to verify this value.
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 reply as above
@@ -30,7 +32,8 @@ | |||
"0.11.1": "0x9b1eF52DdEc3b8414FbE359ed7826334729ab97E", | |||
"0.12.0-rc.0": "0xA20454137b47440C71fE4DD203D25D69F0b34535", | |||
"0.12.0": "0x4555ed733f58Da8Cc6A2Fd6A050874192Ac97985", | |||
"0.12.1": "0xA5467dfe7019bF2C7C5F7A707711B9d4cAD118c8" | |||
"0.12.1": "0xA5467dfe7019bF2C7C5F7A707711B9d4cAD118c8", | |||
"0.14.0": "0x7870C5B8BC9572A8001C3f96f7ff59961B23500D" |
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.
How/where to verify these contracts values?
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.
How/where to verify these contracts values?
These are the addresses of the latest contracts verified deployed + verified by the CI of the @lukso/lsp-smart-contracts
repo on testnet
https://github.com/lukso-network/lsp-smart-contracts/actions/runs/7113905576
@lukso/lsp-smart-contracts
to v0.14.0@lukso/lsp-smart-contracts
to v0.14.0
What kind of change does this PR introduce (bug fix, feature, docs update, ...)?
Package dependency upgrade
@erc725/erc725.js
to new version v0.22.0.tokenType
on deployment of Digital Assets LSP7/8, related to the new LSP4 deployment parameter.tokenIdType
totokenIdFormat