Skip to content
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

Sygma Client implementation. #923

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Sygma Client implementation. #923

wants to merge 18 commits into from

Conversation

kantagara
Copy link
Contributor

@kantagara kantagara commented Apr 12, 2024

FINALLY sygma works.
IT took a LOT of time to make it work.

One very very important note: I am pretty sure that for all of you the Erc1155 will fail to execute. You'd need to own some 1155 with a specific tokenId on Sepolia, and then you can test it out. Chain Id is set to Cronos.

Guide to test:

  • Own some token for 1155

  • In SampleMain, go to Category - Sygma

  • There is sygma client script attached to it

  • Adjust tokenId, other things should remain as is I'd say

  • If everything works you can check the transaction by going to
    https://sepolia.etherscan.io/tx/TRANSACTION_HASH

  • Where TRANSACTION_HASH is the hash that got spit out as output from SygmaClient-SendTransaction
    image

ANOTHER NOTE:
I did update Countly on this branch by accident, so just ignore everything Countly related.

@kantagara kantagara added the ready-to-merge Ready to Merge PR - this'll trigger required checks label Apr 12, 2024
@kantagara kantagara added ready-to-merge Ready to Merge PR - this'll trigger required checks and removed ready-to-merge Ready to Merge PR - this'll trigger required checks labels Apr 12, 2024
@kantagara kantagara added ready-to-merge Ready to Merge PR - this'll trigger required checks and removed ready-to-merge Ready to Merge PR - this'll trigger required checks labels Apr 12, 2024
@kantagara kantagara added ready-to-merge Ready to Merge PR - this'll trigger required checks and removed ready-to-merge Ready to Merge PR - this'll trigger required checks labels Apr 12, 2024
Copy link
Member

boorich commented Apr 15, 2024

This is great!

Copy link
Contributor

@juans-chainsafe juans-chainsafe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some updates:
The flow is working correctly in our side right now, I tested in the editor with wallet connect in Metamask and in WebGL with Metamask login.

But there is a issue in Sygma side right now that causes the transfer to not go through.
I will test the complete flow when thats fixed.

FYI: something weird that Im not sure if is in our side or Sygma, the approve to access the tokens inside the collection should happen only once (if that collection was approved before), otherwise we are spending extra gas fees on that every time https://sepolia.etherscan.io/tx/0x9143b7d169f8c899012f66828bbe1fe7c41518dc50bd4a0d30121e7a41e3d07f cc @kantagara if is on our side, we can have a separate ticket for this

Copy link
Contributor

@juans-chainsafe juans-chainsafe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality is working if you provide the specific data to use it, but for a final user is not practical.

A final version will need:

  • Approve only 1 time per collection and not in all transactions
  • A log for the user to know where he can find the token in the new chain or the transaction hash in that chain
  • Be able to select which tokens I want to transfer
  • Should work with any ERC-1155 tokens and not only the ones that Sygma send to us

But this implementation is a big step and a great functionality to have! Congrats

Copy link
Contributor

@oleksandrchainsafe oleksandrchainsafe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of comments

@@ -19,22 +20,23 @@ public class BasicFeeHandler : IBasicHandler
public BasicFeeHandler(IContractBuilder cb, string address)
{
this.address = address;
this.contract = cb.Build(FeeHandlerAbi, address);
contract = cb.Build(FeeHandlerAbi, address);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FeeHandlerAbi, you can replace the huge constant declared inside the file with an embedded resource. There is an example in the Marketplace project.
There is also a helper method to read ABI from resources. It's called AbiHelper.ReadAbiFromResources.


namespace ChainSafe.Gaming.SygmaClient.DepositDataHandlers
{
// Struct so we don't allocate memory on the heap
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a struct.

throw new Web3Exception("Web3 Instance is not set. Please set it after you've built the Web3 instance.");
}

await Erc20.TransferErc20(Web3Accessor.Web3, Contracts.Erc20, toAddress, System.Numerics.BigInteger.Parse(amount));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this sample have anything to do with the Sygma integration?


private ISygmaClient _sygmaClient;
private bool _isSygmaInitialized;
private Web3 _web3;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were using lowercase names without the "_" prefix for private fields before. Let's stick to one style.

[field: SerializeField] public uint Amount { get; private set; } = 1;
}

public class SygmaClient : MonoBehaviour
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend renaming it to avoid confusion around names. This is more like SygmaSample that Client.

using Environment = ChainSafe.Gaming.SygmaClient.Types.Environment;

[Serializable]
public class SygmaConfig
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can move all fields right into the sample behaviour, as this is more like SygmaSampleConfig than the actual config of the SygmaClient.

button.onClick.AddListener(CallSygma);
_web3 = Web3Accessor.Web3;
_sygmaClient = Web3Accessor.Web3.SygmaClient();
_isSygmaInitialized = _sygmaClient.Initialize(sygmaConfig.SygmaEnvironment);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the value of SygmaEnvironment reflect the one for the original (default) chain settings from the server settings window?

Copy link
Collaborator

@rob1997 rob1997 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! added a few comments, mostly just questions tbh

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to track these DLLs, they'll be auto committed in dev

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or the DLLs in Packages/io.chainsafe.web3-unity/Runtime/Libraries/

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these our own edits to the plugin or just an update?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Sygma necessarily need it's own scene? Can't we add it in a Column in SampleMain?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can ignore this, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Ready to Merge PR - this'll trigger required checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants