Problem: missing dynamic contract (fix #226) #230

leejw51crypto · 2022-12-13T05:27:01Z

Solution: add dynamic contract

leejw51crypto · 2022-12-13T05:29:07Z

files under CronosPlayUnreal can be reviewed

other files are automatically generated

leejw51crypto · 2022-12-13T05:31:15Z

checking ci issues

leejw51crypto · 2022-12-13T06:21:31Z

investigating unreal error

leejw51crypto · 2022-12-13T07:01:09Z

@damoncro fixed in play-cpp-sdk, will upgrade play-cpp-sdk shortly

leejw51crypto · 2022-12-13T07:25:14Z

for linux, back to use clang 10

tomtau

a few unrelated linter errors (/Source/CronosPlayUnreal/Private/DefiWalletCoreActor.cpp:20:13 [modernize-use-trailing-return-type] use a trailing return type for this function; /Source/CronosPlayUnreal/Private/DefiWalletCoreActor.cpp:28:21 [cppcoreguidelines-init-variables] variable 'ret' is not initialized etc.)

--

besides that, can the API in Unreal be split into contract object construction (to be done once by the developer) and function calling (that could be done multiple times) to match C++ bindings?

tomtau · 2022-12-13T08:22:03Z

Source/CronosPlayUnreal/Private/DefiWalletCoreActor.cpp

+void ADefiWalletCoreActor::SendDynamicContract(
+    FString contractAddress, int32 walletindex, FString abiJson,
+    FString functionName, FString functionArgs,
+    FDynamicContractSendDelegate Out) {
+


should these all be under defiwalletcoreactor? or could there be a "contract actor" or something like that that one will initialize once with the common params (contract address, abijson, rpc address) and then just call a function on it?

yes, that's possible. i'll refactor

tomtau · 2022-12-13T08:26:52Z

Source/CronosPlayUnreal/Public/DefiWalletCoreActor.h

+     * @param walletindex wallet index which starts from 0
+     * @param abiJson abi json
+     * @param functionName function name
+     * @param functionArgs function args injson


for the function args, it'll be good to give an example, because the format is not obvious just from the description... maybe using doxygen's \code ... \endcode command?

ok, i'll add an example , so that user can refer to it

leejw51crypto · 2022-12-13T08:50:15Z

ok, working on the feedback

leejw51crypto · 2022-12-14T02:31:22Z

a few unrelated linter errors (/Source/CronosPlayUnreal/Private/DefiWalletCoreActor.cpp:20:13 [modernize-use-trailing-return-type] use a trailing return type for this function; /Source/CronosPlayUnreal/Private/DefiWalletCoreActor.cpp:28:21 [cppcoreguidelines-init-variables] variable 'ret' is not initialized etc.)

--

besides that, can the API in Unreal be split into contract object construction (to be done once by the developer) and function calling (that could be done multiple times) to match C++ bindings?

yes, that's possible.
i'll separate DynamicContract as object, can be called multiple times

leejw51crypto · 2022-12-14T02:32:57Z

working now~

leejw51crypto · 2022-12-14T08:06:26Z

added DynamicContract actor,
ported send, encode, call to this class
testing now

leejw51crypto · 2022-12-14T11:36:06Z

rebased

leejw51crypto · 2022-12-14T12:06:23Z

uninitialize error : using default constructor doesn't work, applied NOLINT

fixed trailing return warning

tomtau

can the actor be instantiated multiple times (if one wanted to use different dynamic contracts at the same time)?

Source/CronosPlayUnreal/Private/DefiWalletCoreActor.cpp

tomtau · 2022-12-15T01:56:10Z

Source/CronosPlayUnreal/Private/DynamicContractActor.cpp

@@ -0,0 +1,241 @@
+// Fill out your copyright notice in the Description page of Project Settings.


I guess this comment should be changed?

yes, i'll remove it

tomtau · 2022-12-15T01:56:42Z

Source/CronosPlayUnreal/Private/DynamicContractActor.cpp

+    // Set this actor to call Tick() every frame.  You can turn this off to
+    // improve performance if you don't need it.
+    PrimaryActorTick.bCanEverTick = true;


should this be off?

tomtau · 2022-12-15T01:57:31Z

Source/CronosPlayUnreal/Private/DynamicContractActor.cpp

+    // improve performance if you don't need it.
+    PrimaryActorTick.bCanEverTick = true;
+    defiWallet = NULL;
+    _coreContract = NULL;


so it will only allow one contract globally? or can one instantiate multiple actors that have different contracts?

it can be multiple

tomtau · 2022-12-15T01:58:35Z

Source/CronosPlayUnreal/Public/DynamicContractActor.h

@@ -0,0 +1,161 @@
+// Fill out your copyright notice in the Description page of Project Settings.


this is probably to be changed?

yes, i'll remove it

tomtau · 2022-12-15T01:59:27Z

Source/CronosPlayUnreal/Public/DynamicContractActor.h

+    /**
+     * @param cronosrpc    cronos rpc server endpoint
+     * @param contractaddress    contract address
+     * @param abijson    contract abi json


maybe it should specify that it should be the abi json text/content... someone may be confused and put e.g. a file path to abi json?

tomtau · 2022-12-15T02:01:28Z

Source/CronosPlayUnreal/Public/DynamicContractActor.h

+     * simple json example: Address, String
+     * [{"Address":{"data":"0x00"}},{"Str":{"data":"my"}}]


for these examples, maybe it'll be also good to write Solidity function signature, so it's more obvious what t hat sample corresponds?

damoncro · 2022-12-15T02:28:19Z

CI does not run.

leejw51crypto · 2022-12-15T09:57:31Z

working on feedback, and will check ci

leejw51crypto · 2022-12-16T06:20:39Z

updated changelog

damoncro · 2022-12-16T06:40:52Z

Any reason to create a new Actor instead of putting it into DefiWalletCoreActor? Common things like cronosrpc can be shared inside DefiWalletCoreActor.

leejw51crypto · 2022-12-19T02:23:18Z

dynamic contract can be created and destroyed easily.
and dynamic contract has its own context, can call functions multiple times.
and it's connected by smart pointer, so signing is done from defi-wallet-core.
with non-modifying calls, defi-wallet-core is not necessary

damoncro · 2022-12-20T03:29:35Z

dynamic contract can be created and destroyed easily.

and dynamic contract has its own context, can call functions multiple times.

and it's connected by smart pointer, so signing is done from defi-wallet-core.

with non-modifying calls, defi-wallet-core is not necessary

How about cronosrpc?

Source/CronosPlayUnreal/Private/DynamicContractActor.cpp

Source/CronosPlayUnreal/Public/DynamicContractActor.h

leejw51crypto · 2022-12-20T13:26:25Z

working on it

leejw51crypto · 2023-01-03T08:56:32Z

ready to review

tomtau

one main question is why *_coreContract is public? I guess *defiWallet needs to be public, so that it can be set/accessed from a blueprint? or could it be private and have blueprint getter/setter?

tomtau · 2023-01-04T02:52:57Z

Source/CronosPlayUnreal/Private/DefiWalletCoreActor.cpp

+                                            FString abijson, bool &success,
+                                            FString &output_message) {
+    UDynamicContractObject *ret = NewObject<UDynamicContractObject>();
+    ret->defiWallet = this;


is it ok for ret->defiWallet to be accessed concurrently?

raw pointer is not protected, but defiWallet is marked as "UPROPERTY" , and managed by unreal garbage collector.

and like unity3d, for unreal engine,engine related apis are all in the single thread(game thread), so concurrency safe. only ue4 engine apis can be called in that thread.

rendering, networking are in other threads

_coreContract can be private, i'll change

i'm double checking with unreal engine document

tomtau · 2023-01-04T02:55:50Z

Source/CronosPlayUnreal/Private/DynamicContractObject.cpp

+        // restored back
+        rust::cxxbridge1::Box<org::defi_wallet_core::EthContract> tmpwallet =
+            rust::cxxbridge1::Box<org::defi_wallet_core::EthContract>::from_raw(
+                _coreContract);


why is this needed? for deallocating that contract object in Rust?
nit: var name perhaps can be tmpcontract?

c++ pointer is earned by "into_raw" , so give ownership back to rust box, remove elegantly.

i'll change the name

tomtau · 2023-01-04T02:59:20Z

Source/CronosPlayUnreal/Public/DynamicContractObject.h

+#include "PlayCppSdkLibrary/Include/defi-wallet-core-cpp/src/ethereum.rs.h"
+#include "DynamicContractObject.generated.h"
+
+using namespace std;


there are some linter warnings:

variable 'namespace' is non-const and globally accessible, consider making it const

ok, i'll check linting errors

tomtau · 2023-01-04T03:01:30Z

Source/CronosPlayUnreal/Public/DynamicContractObject.h

+    UPROPERTY(EditAnywhere, BlueprintReadWrite, Category = "CronosPlayUnreal")
+    ADefiWalletCoreActor *defiWallet;
+
+    org::defi_wallet_core::EthContract *_coreContract;


why to have this internal field in public: and not private: ?

_coreContract can be private, i'll change

leejw51crypto · 2023-01-04T07:28:13Z

fixed c++ linting errors

leejw51crypto · 2023-01-05T01:06:22Z

checked UPROERTY and raw pointer
ref:
https://docs.unrealengine.com/4.27/en-US/ProgrammingAndScripting/ProgrammingWithCPP/UnrealArchitecture/Objects/Optimizations/

document is a little ambiguous

damoncro · 2023-01-05T04:42:50Z

Please update CHANGELOG.md.

leejw51crypto · 2023-01-05T05:04:05Z

ok

update play-cpp-sdk ok add encode add call adding send send working add read json add comment format back format rebase specify ubuntu version use ubuntu-20.04 fix signing upgrade play-cpp-sdk compiles send ok read json as static add comment add json example add example fix typo restore Problem: missing dynamic contract (fix #226) update play-cpp-sdk ok add encode add call adding send send working add read json add comment format back format rebase specify ubuntu version use ubuntu-20.04 fix signing upgrade play-cpp-sdk compiles send ok read json as static add comment add json example add example fix typo restore check lint update play-cpp-sdk change header not run in every frame json string clear comment add function sig add changelog add dynamic-contract uobject works add log add log remove contract actor add comment add comment add json example add json comment make private change variable name fix lint nolint changelog update

leejw51crypto requested review from tomtau and damoncro December 13, 2022 05:27

leejw51crypto requested a review from ricky321q December 13, 2022 05:33

leejw51crypto force-pushed the feature/226a branch from ddb449f to a011629 Compare December 13, 2022 06:52

tomtau reviewed Dec 13, 2022

View reviewed changes

leejw51crypto marked this pull request as draft December 13, 2022 08:50

leejw51crypto force-pushed the feature/226a branch from 350b2f7 to 768d127 Compare December 13, 2022 09:14

leejw51crypto force-pushed the feature/226a branch 2 times, most recently from b89c127 to c8804ff Compare December 14, 2022 11:35

leejw51crypto force-pushed the feature/226a branch 2 times, most recently from df622d6 to 24b48a8 Compare December 14, 2022 11:41

leejw51crypto force-pushed the feature/226a branch from 1e956ea to 1771298 Compare December 14, 2022 15:01

leejw51crypto marked this pull request as ready for review December 14, 2022 15:02

leejw51crypto marked this pull request as draft December 15, 2022 01:32

tomtau reviewed Dec 15, 2022

View reviewed changes

leejw51crypto force-pushed the feature/226a branch 2 times, most recently from 19ea96d to 5d1eb5b Compare December 16, 2022 05:55

leejw51crypto force-pushed the feature/226a branch from 6ce0536 to 2e3eb3f Compare December 20, 2022 03:24

damoncro reviewed Dec 20, 2022

View reviewed changes

Source/CronosPlayUnreal/Private/DynamicContractActor.cpp Outdated Show resolved Hide resolved

damoncro reviewed Dec 20, 2022

View reviewed changes

Source/CronosPlayUnreal/Private/DynamicContractActor.cpp Outdated Show resolved Hide resolved

damoncro reviewed Dec 20, 2022

View reviewed changes

Source/CronosPlayUnreal/Private/DynamicContractActor.cpp Outdated Show resolved Hide resolved

damoncro suggested changes Dec 20, 2022

View reviewed changes

Source/CronosPlayUnreal/Public/DynamicContractActor.h Outdated Show resolved Hide resolved

Source/CronosPlayUnreal/Public/DynamicContractActor.h Outdated Show resolved Hide resolved

Source/CronosPlayUnreal/Public/DynamicContractActor.h Outdated Show resolved Hide resolved

leejw51crypto force-pushed the feature/226a branch 4 times, most recently from 58148b9 to 38eca15 Compare January 3, 2023 08:52

leejw51crypto requested a review from damoncro January 3, 2023 08:53

leejw51crypto force-pushed the feature/226a branch from 38eca15 to e885e31 Compare January 3, 2023 09:09

tomtau reviewed Jan 4, 2023

View reviewed changes

damoncro approved these changes Jan 4, 2023

View reviewed changes

leejw51crypto force-pushed the feature/226a branch from e885e31 to 264a125 Compare January 4, 2023 08:57

leejw51crypto marked this pull request as draft January 5, 2023 01:06

leejw51crypto marked this pull request as ready for review January 5, 2023 01:30

leejw51crypto force-pushed the feature/226a branch from 264a125 to 04813a8 Compare January 5, 2023 05:47

tomtau merged commit 3c54ad5 into main Jan 5, 2023

tomtau deleted the feature/226a branch February 9, 2023 06:41

		@@ -0,0 +1,241 @@
		// Fill out your copyright notice in the Description page of Project Settings.

		@@ -0,0 +1,161 @@
		// Fill out your copyright notice in the Description page of Project Settings.

		* simple json example: Address, String
		* [{"Address":{"data":"0x00"}},{"Str":{"data":"my"}}]

Problem: missing dynamic contract (fix #226) #230

Problem: missing dynamic contract (fix #226) #230

Conversation

leejw51crypto commented Dec 13, 2022

leejw51crypto commented Dec 13, 2022

leejw51crypto commented Dec 13, 2022

leejw51crypto commented Dec 13, 2022

leejw51crypto commented Dec 13, 2022 • edited Loading

leejw51crypto commented Dec 13, 2022

tomtau left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejw51crypto commented Dec 13, 2022

leejw51crypto commented Dec 14, 2022

leejw51crypto commented Dec 14, 2022

leejw51crypto commented Dec 14, 2022

leejw51crypto commented Dec 14, 2022

leejw51crypto commented Dec 14, 2022 • edited Loading

tomtau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damoncro commented Dec 15, 2022

leejw51crypto commented Dec 15, 2022 • edited Loading

leejw51crypto commented Dec 16, 2022

damoncro commented Dec 16, 2022 • edited Loading

leejw51crypto commented Dec 19, 2022 • edited Loading

damoncro commented Dec 20, 2022

leejw51crypto commented Dec 20, 2022

leejw51crypto commented Jan 3, 2023

tomtau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejw51crypto Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejw51crypto Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejw51crypto commented Jan 4, 2023 • edited Loading

leejw51crypto commented Jan 5, 2023 • edited Loading

damoncro commented Jan 5, 2023 • edited Loading

leejw51crypto commented Jan 5, 2023

leejw51crypto commented Dec 13, 2022 •

edited

Loading

tomtau left a comment •

edited

Loading

leejw51crypto commented Dec 14, 2022 •

edited

Loading

leejw51crypto commented Dec 15, 2022 •

edited

Loading

damoncro commented Dec 16, 2022 •

edited

Loading

leejw51crypto commented Dec 19, 2022 •

edited

Loading

leejw51crypto Jan 4, 2023 •

edited

Loading

leejw51crypto Jan 4, 2023 •

edited

Loading

leejw51crypto commented Jan 4, 2023 •

edited

Loading

leejw51crypto commented Jan 5, 2023 •

edited

Loading

damoncro commented Jan 5, 2023 •

edited

Loading