-
Notifications
You must be signed in to change notification settings - Fork 13
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
Remove Goerli chain ID support #454
Remove Goerli chain ID support #454
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
@@ -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 |
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.
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?
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.
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)
// 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) |
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.
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.
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 also applies to other places where this line is present
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") |
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 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.
@@ -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) { |
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.
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
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.
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
lib/src/test/kotlin/com/swmansion/starknet/data/TransactionHashCalculatorTest.kt
Outdated
Show resolved
Hide resolved
@@ -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) |
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.
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
Co-authored-by: Wojciech Szymczyk <wojciech.szymczyk@swmansion.com>
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.
LGTM overall
lib/src/test/kotlin/com/swmansion/starknet/data/TransactionHashCalculatorTest.kt
Outdated
Show resolved
Hide resolved
… into chore/425-remove-goerli-chain-id-support
Describe your changes
chainId
to useStarknetChainId.fromNetworkName("SN_GOERLI")
inDeprecatedTransactionHashTest
andTransactionHashV3Test
- we just want to test calculation process, so there's no need to update this field to SepoliaLinked issues
Closes #425
Breaking changes
StarknetChainId
: RemovedGOERLI
field