Remove Goerli chain ID support #454

franciszekjob · 2024-04-15T11:07:11Z

Describe your changes

Bump starknet-devnet-rs to v0.0.5
Change Goerli to Sepolia in references and tests
Update chainId to use StarknetChainId.fromNetworkName("SN_GOERLI") in DeprecatedTransactionHashTest and TransactionHashV3Test - we just want to test calculation process, so there's no need to update this field to Sepolia
Remove unused secrets

Linked issues

Closes #425

Breaking changes

This issue contains breaking changes
StarknetChainId: Removed GOERLI field

codecov-commenter · 2024-04-17T11:10:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.60%. Comparing base (d28b008) to head (70c5f16).
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #454      +/-   ##
==========================================
- Coverage   71.12%   69.60%   -1.52%     
==========================================
  Files          75       75              
  Lines        3338     3313      -25     
  Branches      367      365       -2     
==========================================
- Hits         2374     2306      -68     
- Misses        798      848      +50     
+ Partials      166      159       -7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

.github/workflows/checks.yml

README.md

DelevoXDG · 2024-04-17T13:03:46Z

lib/src/test/kotlin/com/swmansion/starknet/data/TransactionHashCalculatorTest.kt

@@ -12,7 +12,7 @@ internal class TransactionHashCalculatorTest {
    inner class DeprecatedTransactionHashTest {
        private val calldata = listOf(Felt(999), Felt(888), Felt(777))
        private val maxFee = Felt.fromHex("0xabcd987654210")
-        private val chainId = StarknetChainId.GOERLI
+        private val chainId = StarknetChainId.SEPOLIA


Nit: I'm a bit unsure about this change. If I remember correctly, the previous hashes were verified against some tool like sn.js.

Did you verify the new ones? Maybe we should just use StarknetChainId.fromHex("SN_GOERLI") to avoid changing the test cases.

@THenry14 WDYT?

I think we should be pretty safe, we can verify to be double-sure but I suppose this should be fine as is (as we do not change any logic behind calculations)

DelevoXDG · 2024-04-17T13:09:40Z

lib/src/test/kotlin/network/account/AccountTest.kt

        // This test sometimes fails due to getNonce receiving higher (pending) nonce than addDeclareTransaction expects

        // TODO: (#384) Test v3 transactions on Sepolia
-        assumeTrue(network == Network.GOERLI_INTEGRATION)
+        assumeTrue(network == Network.SEPOLIA_INTEGRATION)


Does this test pass locally?

If I remember correctly, v3 tests on Sepolia networks should've been added only in #384. If they do work as expected on SEPOLIA_INTEGRATION without any additional changes, I would assume SEPOLIA_TESTNET tests should work as well and this line could be removed.

This also applies to other places where this line is present

DelevoXDG · 2024-04-17T13:18:46Z

lib/src/test/kotlin/network/provider/ProviderTest.kt

            Network.SEPOLIA_INTEGRATION -> Felt.fromHex("0x01556b67a22bc26dfc4827286997e3e3b53380e14ba1d879f1d67b7ffbd5a808")
            Network.SEPOLIA_TESTNET -> Felt.fromHex("0x0683e900e4c0e27e164f0b7569f86edec6dec919e55d97728113b40fe6b731c7")
        }
        private val deployAccountV3TransactionHash by lazy {
            when (network) {
-                Network.GOERLI_INTEGRATION -> Felt.fromHex("0x7c1ca558aaec1a14a4c0553517013631fad81c48667a3bcd635617c2560276")
+                Network.SEPOLIA_INTEGRATION -> Felt.fromHex("0x7c1ca558aaec1a14a4c0553517013631fad81c48667a3bcd635617c2560276")


This will not work, there's no such transaction on Sepolia integration.

You should manually look up deploy account / invoke / declare v3 transactions for both testnet and integration Sepolia networks (or send them yourself) and paste their hashes here.

This is of course part of a different issue (#384), so you can either include the change in this PR, or disable tests for now and deal with this in a separate one.

DelevoXDG · 2024-04-17T13:19:29Z

lib/src/test/kotlin/network/provider/ProviderTest.kt

@@ -121,7 +105,7 @@ class ProviderTest {
        val transactionHash2 = revertedInvokeV1TransactionHash
        val transactionStatus = provider.getTransactionStatus(transactionHash).send()
        // TODO: Re-enable this assertion for integration once transaction appear as accepted on L1 again
-        if (network != Network.GOERLI_INTEGRATION) {
+        if (network != Network.SEPOLIA_INTEGRATION) {


Please verify whether this is still needed on Sepolia integration.

Co-authored-by: Maksim Zdobnikau <43750648+DelevoXDG@users.noreply.github.com>

…thub.com/software-mansion/starknet-jvm into chore/425-remove-goerli-chain-id-support

DelevoXDG

Discussed changes offline, submitting empty review so you can re-request it later

…thub.com/software-mansion/starknet-jvm into chore/425-remove-goerli-chain-id-support

…heck

lib/src/test/kotlin/com/swmansion/starknet/data/TransactionHashCalculatorTest.kt

DelevoXDG · 2024-05-16T10:37:27Z

lib/src/test/kotlin/network/account/AccountTest.kt

@@ -766,12 +756,10 @@ class AccountTest {
    fun `test udc deploy with parameters`() {
        assumeTrue(NetworkConfig.isTestEnabled(requiresGas = true))

-        assumeFalse(network == Network.GOERLI_INTEGRATION)
+        assumeFalse(network == Network.SEPOLIA_INTEGRATION)


Is this still needed?

Previously, it was used because (for whatever reason) we could only run the test on GOERLI_INTEGRATION. Does that still hold and this test pass only on SEPOLIA_INTEGRATION?

The same applies to all the other assumeFalse / assumeTrue checks

…hCalculatorTest.kt Co-authored-by: Maksim Zdobnikau <43750648+DelevoXDG@users.noreply.github.com>

…thub.com/software-mansion/starknet-jvm into chore/425-remove-goerli-chain-id-support

… into chore/425-remove-goerli-chain-id-support

.github/workflows/checks.yml

README.md

androiddemo/README.md

lib/src/test/kotlin/network/account/AccountTest.kt

Co-authored-by: Wojciech Szymczyk <wojciech.szymczyk@swmansion.com>

DelevoXDG

LGTM overall

.github/workflows/checks.yml

README.md

lib/src/test/kotlin/com/swmansion/starknet/data/TransactionHashCalculatorTest.kt

lib/src/test/kotlin/network/account/AccountTest.kt

…OERLI")`

… into chore/425-remove-goerli-chain-id-support

franciszekjob added 13 commits April 15, 2024 11:37

Remove Goerli chain ID references

82e867e

Update

003d6ed

Rename alpha-goerli to alpha-sepolia in test accounts

5b90b89

Format

2c66cb1

Fix comments

91d4fdc

Update devnet version

d2cae1b

Update devnet version

3ff74bd

Rename network from alpha-sepolia to SN_SEPOLIA

89a4465

Update expected values in transaction hash calculation tests

67d7ec3

Update expected transaction hash in getHash test

b47c29a

Update expected tx2 hash in getHash test

6e87795

Change requested blockNumber in test

b7f5953

Updated expected test value

b98f8a5

Update testnet funds faucet url

404b373

franciszekjob requested a review from DelevoXDG April 17, 2024 12:18

DelevoXDG reviewed Apr 17, 2024

View reviewed changes

franciszekjob and others added 8 commits April 17, 2024 15:34

Update README.md

3d9e9cf

Co-authored-by: Maksim Zdobnikau <43750648+DelevoXDG@users.noreply.github.com>

Restore Goerli tx hashes in DeprecatedTransactionHashTest

95f4116

Merge branch 'chore/425-remove-goerli-chain-id-support' of https://gi…

75aded5

…thub.com/software-mansion/starknet-jvm into chore/425-remove-goerli-chain-id-support

Disable TransactionHashCalculatorTest

7102305

Format

0b8f0fa

Disable tests

6b4f060

Disable network tests

0825990

Remove network-related secrets

0409216

franciszekjob requested review from THenry14 and DelevoXDG April 19, 2024 08:48

DelevoXDG reviewed Apr 19, 2024

View reviewed changes

franciszekjob requested a review from DelevoXDG April 19, 2024 17:00

DelevoXDG removed their request for review April 20, 2024 09:33

franciszekjob added 3 commits April 23, 2024 10:06

Merge branch 'chore/425-remove-goerli-chain-id-support' of https://gi…

f774dda

…thub.com/software-mansion/starknet-jvm into chore/425-remove-goerli-chain-id-support

Change chainId to StarknetChainId.fromNetworkName("SN_GOERLI")

5b8a44f

Format

6ba4326

franciszekjob requested a review from DelevoXDG April 23, 2024 13:40

franciszekjob added 10 commits April 23, 2024 22:06

Change chain key to alpha-sepolia in account jsons

4d5c1ea

Remove --path-to-scarb-toml flag from sncast process builder

7ecb35e

Update tests

5fa883c

Format

e6d54c9

Restore chain key to SN_SEPOLIA in account jsons

b944782

Remove unnecessary DevnetClient change

6720021

Update transaction hashes in TransactionHashCalculatorTest

99e8abb

Update transaction hashes in Provider tests

65c7e06

Re-enable assertions for TransactionFinalityStatus.ACCEPTED_ON_L1 c…

535fec7

…heck

Update tx hash in get transaction receipt with l2 to l1 messages test

366ce19

DelevoXDG requested changes May 16, 2024

View reviewed changes

franciszekjob and others added 3 commits May 16, 2024 14:56

Update lib/src/test/kotlin/com/swmansion/starknet/data/TransactionHas…

66a63d5

…hCalculatorTest.kt Co-authored-by: Maksim Zdobnikau <43750648+DelevoXDG@users.noreply.github.com>

Remove network assumption from test udc deploy with parameters

9f9ee0e

Merge branch 'chore/425-remove-goerli-chain-id-support' of https://gi…

c6537dd

…thub.com/software-mansion/starknet-jvm into chore/425-remove-goerli-chain-id-support

franciszekjob requested a review from DelevoXDG May 16, 2024 14:41

Merge branch 'main' of https://github.com/software-mansion/starknet-jvm…

432594b

… into chore/425-remove-goerli-chain-id-support

franciszekjob removed the request for review from THenry14 May 17, 2024 18:14

THenry14 reviewed May 20, 2024

View reviewed changes

Update lib/src/test/kotlin/network/account/AccountTest.kt

13d70db

Co-authored-by: Wojciech Szymczyk <wojciech.szymczyk@swmansion.com>

franciszekjob requested a review from THenry14 May 20, 2024 09:22

DelevoXDG approved these changes May 20, 2024

View reviewed changes

franciszekjob added 4 commits May 20, 2024 16:17

Restore hashes in TransactionHashV3Test; Use `fromNetworkName("SN_G…

b806c1e

…OERLI")`

Merge branch 'main' of https://github.com/software-mansion/starknet-jvm…

f3233c6

… into chore/425-remove-goerli-chain-id-support

Update readme

555df54

Update readme

70c5f16

franciszekjob merged commit 586897f into main May 20, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Goerli chain ID support #454

Remove Goerli chain ID support #454

franciszekjob commented Apr 15, 2024 •

edited

codecov-commenter commented Apr 17, 2024 •

edited

DelevoXDG Apr 17, 2024

THenry14 Apr 18, 2024

DelevoXDG Apr 17, 2024

DelevoXDG Apr 17, 2024

DelevoXDG Apr 17, 2024

DelevoXDG Apr 17, 2024 •

edited

DelevoXDG left a comment

DelevoXDG May 16, 2024

DelevoXDG left a comment

Remove Goerli chain ID support #454

Remove Goerli chain ID support #454

Conversation

franciszekjob commented Apr 15, 2024 • edited

Describe your changes

Linked issues

Breaking changes

codecov-commenter commented Apr 17, 2024 • edited

Codecov Report

DelevoXDG Apr 17, 2024

Choose a reason for hiding this comment

THenry14 Apr 18, 2024

Choose a reason for hiding this comment

DelevoXDG Apr 17, 2024

Choose a reason for hiding this comment

DelevoXDG Apr 17, 2024

Choose a reason for hiding this comment

DelevoXDG Apr 17, 2024

Choose a reason for hiding this comment

DelevoXDG Apr 17, 2024 • edited

Choose a reason for hiding this comment

DelevoXDG left a comment

Choose a reason for hiding this comment

DelevoXDG May 16, 2024

Choose a reason for hiding this comment

DelevoXDG left a comment

Choose a reason for hiding this comment

franciszekjob commented Apr 15, 2024 •

edited

codecov-commenter commented Apr 17, 2024 •

edited

DelevoXDG Apr 17, 2024 •

edited