-
Notifications
You must be signed in to change notification settings - Fork 2
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
Created Contract Wrappers #183
base: master
Are you sure you want to change the base?
Conversation
Created Contract wrappers for erc20, erc721, erc1155, with grantrole, mint and burn. Also, erc721Sale, erc1155Sale with "PrimaryPurchase"/mint
small cleanup and renaming
Plugins/SequencePlugin/Source/SequencePlugin/Private/Types/ERC1155.cpp
Outdated
Show resolved
Hide resolved
Plugins/SequencePlugin/Source/SequencePlugin/Private/Types/ERC1155SaleContract.cpp
Outdated
Show resolved
Hide resolved
TArray<uint8> ProofBytes; | ||
ProofBytes.SetNumUninitialized(32); | ||
FMemory::Memcpy(ProofBytes.GetData(), TCHAR_TO_UTF8(*Role), FMath::Min(Role.Len(), 32)); |
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.
When granting a role to a smart contract, you typically grant the role to keccak-256(role_name)
. Is this what this code is doing? If so, we should include it as a keccak256 hash function helper somewhere in the SDK
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 great! Thanks @caballoninja!
A couple of thoughts:
- These are likely to be very high-touch wrappers; we should make sure to add them to our docs (including some sample code)
- Similarly, as these are high-touch points; we should add some tests for them (@ZemindJan @caballoninja would you mind reviewing Tests/session management #172 when you get a chance)
Also, can we add constructors to the contract wrappers to require the contract address? It seems like every method call relies upon that info so I think it would be best for us to force it get set |
revisions on constructors and fixes
What was the update on the GetPaymentToken and GetSaleDetails functions for the sale contract wrappers? Right now theyre removed, or shall we change them to return RawTransactions instead? |
(wouldnt let me answer directly) my idea is to revisit the contract call to make it blueprintable, but if not yes, i will make them rawTx |
Included contractcalls
@andygruening added back the contract 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.
Overall looks good to me. I'll let @andygruening approve as he has more context with this ticket.
Question for you @caballoninja - I noticed that a lot of the contracts have an FString Data field. What is this for?
#include "ERC20.generated.h" | ||
|
||
UCLASS(BlueprintType, Blueprintable) | ||
class SEQUENCEPLUGIN_API UERC20 : public UObject |
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.
Should probably add the Approve function
#include "ERC721.generated.h" | ||
|
||
UCLASS(BlueprintType, Blueprintable) | ||
class SEQUENCEPLUGIN_API UERC721 : public UObject |
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 above, add Approve function
#include "ERC1155.generated.h" | ||
|
||
UCLASS(BlueprintType, Blueprintable) | ||
class SEQUENCEPLUGIN_API UERC1155 : public UObject |
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.
Add Approve function
Created Contract wrappers for erc20, erc721, erc1155, with grantrole, mint and burn. Also, erc721Sale, erc1155Sale with "PrimaryPurchase"/mint