Integrate contracts v2.2.0 #1845

mariacarmina · 2024-09-04T12:08:48Z

Fixes # .

Changes proposed in this PR:

integrated factory contract
updated contract version
created test script for sapphire

alexcos20

see my comments

alexcos20 · 2024-09-11T05:39:38Z

src/@types/NFTFactory.ts

+export interface AccessListData {
+  name: string
+  symbol: string
+  tokenURI: string[]


why do we need this?

it's a typing as NftCreateData is used when creating a NFT.

alexcos20 · 2024-09-11T05:41:37Z

src/contracts/AccessList.ts

@@ -41,6 +39,33 @@ export class AccessListContract extends SmartContract {
    return await accessListContract.tokenURI()


there is no function tokenURI without params, so this will never work

alexcos20 · 2024-09-11T05:42:36Z

src/contracts/AccessList.ts

+    const accessListContract = this.getContract(accessListAddress)
+    return await accessListContract.symbol()
+  }
+
  /**
   * Mint ERC721 contract


mint new token, or add address to accesslist

it's not minting another erc721 contract

I'll modify

alexcos20 · 2024-09-11T05:43:16Z

src/contracts/AccessList.ts

@@ -99,6 +124,13 @@ export class AccessListContract extends SmartContract {
    return <ReceiptOrEstimate<G>>trxReceipt
  }

+  /**
+   * Burn Access List


delete address from accessList or burn tokenId

alexcos20 · 2024-09-11T05:45:02Z

src/contracts/AccessListFactory.ts

+   * @return {Promise<string|BigNumber>} The transaction hash or the gas estimate.
+   */
+  public async deployAccessListContract<G extends boolean = false>(
+    listData: AccessListData,


I would advice you to delete that typing (AccessListData) and use function params

alexcos20 · 2024-09-11T05:47:04Z

src/contracts/NFT.ts

@@ -70,22 +74,32 @@ export class Nft extends SmartContract {
    )
    if (estimateGas) return <G extends false ? string : BigNumber>estGas

+    const addresses = [minter, paymentCollector, mpFeeAddress, feeToken]
+    if (accessListContract) {


only templateID == 4 supports accessLists , so pls make sure that we don't pass it to other template, because those templates might use those fields for something else

alexcos20 · 2024-09-11T05:48:24Z

test/integration/PublishEditConsume.test.ts

-      providerUrl
-    )
-    assert(grapqlOrderTx, 'Ordering graphql dataset failed.')
+    // arwaveOrderTx = await orderAsset(


Out of curiosity, why did we removed this tests?

I have commented them for debug because these were failing.

well, you commented the order , but left the downloads on next tests :) and those are failing without orders..

I can focus on this problem, now since I am blocked on testing the integration side.

alexcos20 · 2024-09-11T05:53:42Z

test/config.ts

@@ -48,3 +48,16 @@ export const getAddresses = () => {
  )
  return data.development
 }
+
+export const getAddressesForSapphire = (testnet: boolean) => {


Maybe it's a good ideea to have two functions:

getAddressesForChain(chainID)

getAddressesForSapphire which calls getAddressesForChain

I have that locally, not pushed yet.

alexcos20 · 2024-09-11T12:58:50Z

why for every call to accesslist functions do i need to provide accessListAddress ?

just have it in the constructor and do something like:

accessList = new AccessListContract('0x123')
owner = await accessList.getOwner()

mariacarmina · 2024-09-11T20:33:23Z

why for every call to accesslist functions do i need to provide accessListAddress ?

just have it in the constructor and do something like:
accessList = new AccessListContract('0x123')
owner = await accessList.getOwner()

It wasn't a specific preference, I can change that AccessList class to extend SmartContractWithAddress class and store the address in constructor. The only reason for using this approach is that Nft class has in each method nft address passed as parameter.

mariacarmina · 2024-09-17T13:31:54Z

The integration tests for ordering different types of assets is an older issue which pops up due to exceeded timeout. I commented the assets which rely on external services such as ipfs and create an issue for fixing those tests in the future since they are not impacted by this PR changes.

Issue created for ipfs which is the only one impacting the tests due to invalid CID: #1849

jamiehewitt15

Why are the sapphire integration tests in a folder called scripts? Seems like it would be better placed in /test/integration/

jamiehewitt15 · 2024-09-18T08:54:24Z

.gitignore

-test/**/*.js
+test/unit/*.js
+test/integration/*.js


Why have you done this? Before we ignored all javascript files in the test directory, now you want only to ignore js files if they are in either /test/unit or test/integration? I don't see any benefit to changing this

It was when I created a .js file, I'll correct it now since I have the .ts script, not .js

jamiehewitt15 · 2024-09-18T08:56:40Z

README.md

@@ -128,6 +128,21 @@ npm run test:integration
 npm run test:integration:cover
 ```

+### Sapphire Integration Tests
+
+Currently, there is used Oasis Sapphire Test network, please export the `PRIVATE_KEY` before testing.


This is worded poorly, I think what you are trying to say is:

Suggested change

Currently, there is used Oasis Sapphire Test network, please export the `PRIVATE_KEY` before testing.

We are currently using the live Oasis Sapphire Test network for the integration tests. Please export the `PRIVATE_KEY` before running the tests.

jamiehewitt15 · 2024-09-18T09:05:10Z

test/scripts/SapphireIntegration.test.ts

+  const provider = sapphire.wrap(
+    ethers.getDefaultProvider(sapphire.NETWORKS.testnet.defaultGateway)
+  )
+  const wallet = new ethers.Wallet(process.env.PRIVATE_KEY, provider)


Given that these tests are using a private key on a live network, at some point, the wallet address is going to run out of tokens and the tests will fail. Is there some sort fo longer-term plan here, like getting the tests to work with barge? If so, do we have an issue for this?

That is the reason that I did not include the tests in the CI, because everytime the account will be drained, we'll have to top up the funds for those. For the moment it's not tested with CI, only locally. We can create an issue for it. @mihaisc suggested to have a separate job which not runs at every push but he said to leave this problem for now, I'll create an issue for it.

ok cool, fair enough

bogdanfazakas · 2024-09-18T10:51:11Z

package.json

@@ -30,6 +30,7 @@
    "mocha": "TS_NODE_PROJECT='./test/tsconfig.json' mocha --config=test/.mocharc.json --node-env=test --exit",
    "test": "npm run lint && npm run test:unit:cover && npm run test:integration:cover",
    "test:unit": "npm run mocha -- 'test/unit/**/*.test.ts'",
+    "test:sapphire": "npm run mocha -- 'test/scripts/**/*.test.ts'",


lets move the test file to its proper location /test/integration/
also renaming it to Sapphire.test.ts would be nice
then update the test:saphire to "npm run mocha -- 'test/integration/Sapphire.test.ts'" .. and then just ignore the newloy moved file in the test:integration comand

Ok, sure I can do that.

bogdanfazakas · 2024-09-18T10:53:50Z

src/contracts/AccessList.ts

+  }
+
+  /**
+   * Add address to access list
   * @param {String} user Minter address
   * @param {String} tokenUri tokenURI
   * @param {Boolean} estimateGas if True, return gas estimate
   * @return {Promise<ReceiptOrEstimate>} transactionId


returns the transaction receipt

bogdanfazakas · 2024-09-18T10:55:30Z

src/contracts/AccessList.ts

+   * Delete address from access list
+   * @param {Number} tokenId token ID
+   * @param {Boolean} estimateGas if True, return gas estimate
+   * @return {Promise<ReceiptOrEstimate>} transactionId


Suggested change

* @return {Promise<ReceiptOrEstimate>} transactionId

* @return {Promise<ReceiptOrEstimate>} returns the transaction receipt or the estimateGas value

bogdanfazakas · 2024-09-18T10:55:48Z

src/contracts/AccessList.ts

      user,
      tokenUri
    )
    return <ReceiptOrEstimate<G>>trxReceipt
  }

  /**
-   * Batch Mint ERC721 contract
-   * @param {String} accessListAddress AccessList contract address
+   * Batch add addresses to the access list
   * @param {String} users Minter addresses
   * @param {String} tokenUris tokenURI
   * @param {Boolean} estimateGas if True, return gas estimate
   * @return {Promise<ReceiptOrEstimate>} transactionId


Suggested change

* @return {Promise<ReceiptOrEstimate>} transactionId

* @return {Promise<ReceiptOrEstimate>} returns the transaction receipt or the estimateGas value

bogdanfazakas · 2024-09-18T10:56:20Z

src/contracts/AccessList.ts

+   * Transfer Ownership of an access list, called by owner of access list
+   * @param {Number} newOwner new owner of the access list
+   * @param {Boolean} estimateGas if True, return gas estimate
+   * @return {Promise<ReceiptOrEstimate>} transactionId


Suggested change

* @return {Promise<ReceiptOrEstimate>} transactionId

* @return {Promise<ReceiptOrEstimate>} returns the transaction receipt or the estimateGas value

bogdanfazakas · 2024-09-18T10:58:59Z

src/contracts/AccessListFactory.ts

+      tokenURI
+    )
+    const trxReceipt = await tx.wait()
+    const events = getEventFromTx(trxReceipt, 'NewAccessList')


what if the tx fails, need to provide a proper response for the client otherwise the line bellow events.args[0] will fail with uncaught error

Yes, the same should be applied in NFTFactory when creating a NFT, try-catch block is missing from there too.

src/utils/Asset.ts

paulo-ocean · 2024-09-18T16:34:29Z

src/config/ConfigHelper.ts

+  {
+    ...configHelperNetworksBase,
+    chainId: 23295,
+    network: 'oasis_saphire_testnet',


this is misspelled again, chain name is: oasis_sapphire_testnet

paulo-ocean · 2024-09-19T09:48:57Z

src/contracts/Datatoken4.ts

+   */
+  public async getFileObject(
+    dtAddress: string,
+    serviceId: number,


We need to refactor all these occurrences of serviceID (including comments).. It definitely is NOT the service id, but the service index.. so serviceIndex instead, otherwise we get easily confused

right thing, sure, do that now.

paulo-ocean · 2024-09-19T09:54:38Z

src/contracts/AccessListFactory.ts

+
+  /**
+   * Create new Access List Contract
+   * @param {AccessListData} listData The data needed to create an NFT.


lets kill this comment about AccessListData :-)

paulo-ocean · 2024-09-19T10:55:31Z

src/contracts/Datatoken4.ts

@@ -30,19 +30,23 @@ export class Datatoken4 extends Datatoken {
    abi?: AbiItem[]
  ) {
    super(signer, network, config, abi)
+    this.abi = this.getDefaultAbi()
    // Wrap signer's address for encrypted data tx
    this.signer = sapphire.wrap(signer)


is the template only for confidential EVM sapphire?
Cause we're wrapping the signer with their sdk call sapphire.wrap(...) ..
and we have a few functions doing it again like:
confidentialEVM === true ? sapphire.wrap(this.signer) : this.signer, ...
So, we get a "double" wrap() for confidential EVM and a wrap() for any other EVM using this template
For now, i would just remove the wrap() call on the constructor, and later, once we merge the other PR we can refactor this a bit more (from the config we would know if is confidential evm and if we should wrap the calls

I can remove the wrapper from constructor to avoid confusions.

Template 4 is only for confidential EVM, I mean, the scope of its creation is destinated for sapphire and files encryption on sapphire chain.

mariacarmina added 3 commits September 4, 2024 10:53

Upgraded contracts v2.2.0.

47500b0

Integrated Access List factory. Created tests.

9695585

Rename file.

fc985b9

mariacarmina self-assigned this Sep 4, 2024

mariacarmina added 14 commits September 4, 2024 15:51

Add script for testing sapphire functionality.

ab9898d

Remove sapphire sdk from accesslist contract.

8e636e5

Added new functions to access list contract.

0924a71

Remove script.

65f5aaa

Remove script.

7653cc2

Increase timeout to the tests.

cec7a26

Added js script. Modified gitignore.

f336192

Install tsx, added oasis testnet into config helpers.

59bdf3d

Modified provider signature.

e8d4749

fix lint.

e2004b6

Fix type module.

2bca54e

commented ordering other assets different than URL.

c2c41e2

Added check for file object for template index.

54b7731

fix deploy erc20 template 4 test.

4125959

alexcos20 requested changes Sep 11, 2024

View reviewed changes

mariacarmina added 7 commits September 11, 2024 12:30

Calculate index function. Fix review.

f97666c

Change to this.signer.

b287294

Uncommented orders.

fe18feb

Upgrade Github actions.

874226c

Condition for calculation of template index.

7b84b3c

Specify ABI in the tests.

da25e32

Test w soulbound contracts.

73988cd

mariacarmina added 2 commits September 11, 2024 16:00

Fixed wallet address for test.

5e96dda

Add deny list check.

2670863

Comment tests with external services.

badeacb

mariacarmina added 5 commits September 17, 2024 17:04

fix graphql URL.

c1b5a48

Insert arweave.

b612061

Insert ipfs.

62c3d56

cleanup. mention ipfs issue.

fb1f02d

Fixes. Added getFilesObject back.

58298e4

jamiehewitt15 reviewed Sep 18, 2024

View reviewed changes

bogdanfazakas reviewed Sep 18, 2024

View reviewed changes

src/utils/Asset.ts Show resolved Hide resolved

paulo-ocean reviewed Sep 18, 2024

View reviewed changes

fix review.

ae1e79f

paulo-ocean reviewed Sep 19, 2024

View reviewed changes

replace srv id w srv index.

f595def

paulo-ocean reviewed Sep 19, 2024

View reviewed changes

Fix doc.

2da79fa

paulo-ocean reviewed Sep 19, 2024

View reviewed changes

mariacarmina added 2 commits September 19, 2024 15:51

Remove wrap call from constructor.

15aaddb

Modify README.

9584d32

mariacarmina requested review from bogdanfazakas, paulo-ocean and jamiehewitt15 September 19, 2024 13:10

		@@ -41,6 +39,33 @@ export class AccessListContract extends SmartContract {
		return await accessListContract.tokenURI()

	Currently, there is used Oasis Sapphire Test network, please export the `PRIVATE_KEY` before testing.
	We are currently using the live Oasis Sapphire Test network for the integration tests. Please export the `PRIVATE_KEY` before running the tests.

	* @return {Promise<ReceiptOrEstimate>} transactionId
	* @return {Promise<ReceiptOrEstimate>} returns the transaction receipt or the estimateGas value

Integrate contracts v2.2.0 #1845

Are you sure you want to change the base?

Integrate contracts v2.2.0 #1845

Conversation

mariacarmina commented Sep 4, 2024 • edited Loading

alexcos20 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcos20 commented Sep 11, 2024

mariacarmina commented Sep 11, 2024

mariacarmina commented Sep 17, 2024 • edited Loading

jamiehewitt15 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

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

mariacarmina commented Sep 4, 2024 •

edited

Loading

mariacarmina commented Sep 17, 2024 •

edited

Loading