-
Notifications
You must be signed in to change notification settings - Fork 37
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
Added methods for dynamic nfts #466
Conversation
|
||
it("should create 'Transaction' for modifying creator", () => { | ||
const transaction = tokenManagementFactory.createTransactionForModifyingCreator({ | ||
sender: grace.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.
Just to double check, only the current creator can do this? If so, who would be the new creator?
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.
from the PR on docs:
The creator of a token can be changed. For this, the token has to be moved to the new creator account. The new creator
account requires `ESDTRoleModifyCreator` role. Also, the token has to be of dynamic type.
sender: grace.address, | ||
tokenIdentifier: "TEST-123456", | ||
tokenNonce: 1n, | ||
royalties: 1234n, |
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.
Maybe we should rename this to newRoyalties
?
}); | ||
|
||
it("should create 'Transaction' for setting new uris", () => { | ||
const transaction = tokenManagementFactory.createTransactionForSettingNewUris({ |
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.
...URIs
sender: grace.address, | ||
tokenIdentifier: "TEST-123456", | ||
tokenNonce: 1n, | ||
uris: ["firstURI", "secondURI"], |
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.
Maybe newURIs
?
tokenNonce: 1n, | ||
tokenName: "Test", | ||
royalties: 1234n, | ||
hash: "abba", | ||
attributes: Buffer.from("test"), | ||
uris: ["firstURI", "secondURI"], |
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.
So that there is no ambiguity regarding what gets changed, we can prefix all the changed fields with "new". E.g. we'll have tokenNonce
, but we'll have newTokenName
.
Additionally, maybe receive a Token
class directly instead of tokenIdentifier
& tokenNonce
? Or maybe not, to keep consistency with existing methods of this factory?
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.
prefixed the fields with new...
according to the specs. Still using tokenIdentifier
and tokenNonce
to be consistent.
@@ -402,6 +418,10 @@ export class TokenManagementTransactionsFactory { | |||
attributes: Uint8Array; | |||
uris: string[]; | |||
}): Transaction { | |||
if (!options.uris.length) { | |||
throw new ErrBadUsage(""); |
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.
Breaking change? Or fixing change? Message is missing.
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 is now deleted.
uris: string[]; | ||
}): Transaction { | ||
if (!options.uris.length) { | ||
throw new ErrBadUsage(""); |
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.
Message is missing.
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
}).build(); | ||
} | ||
|
||
createTransactionForModifyingCreator(options: { |
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.
Who is the new creator?
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.
the one who now owns the token and has the ESDTRoleModifyCreator
role.
royalties: bigint; | ||
hash: string; | ||
attributes: Uint8Array; | ||
uris: string[]; |
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.
Above we have a check on non-empty URIs. Should we have it here, as well?
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'd say no.
royalties: bigint; | ||
hash: string; | ||
attributes: Uint8Array; | ||
uris: string[]; |
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.
Above we have a check on non-empty URIs. Should we have it here, as well?
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'd say no.
addRoleESDTSetNewURI?: boolean; | ||
addRoleESDTModifyRoyalties?: boolean; |
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.
Indeed, optional, to avoid a breaking change 👍
No description provided.