Skip to content

Remove apply_test_resource_bounds_flags#4212

Open
franciszekjob wants to merge 7 commits intomasterfrom
remove-apply-test-resource-bounds
Open

Remove apply_test_resource_bounds_flags#4212
franciszekjob wants to merge 7 commits intomasterfrom
remove-apply-test-resource-bounds

Conversation

@franciszekjob
Copy link
Contributor

@franciszekjob franciszekjob commented Mar 13, 2026

Closes #

Introduced changes

When bumping RPC some time ago, apply_test_resource_bounds was introduced. Because of issues with fee estimation by devnet, we used hard-coded resource bounds. It's not relevant anymore.

Checklist

  • Linked relevant issue
  • Updated relevant documentation
  • Added relevant tests
  • Performed self-review of the code
  • Added changes to CHANGELOG.md

Comment on lines -533 to -536
"--constructor-calldata",
"0x1",
"0x1",
"0x0",
Copy link
Contributor Author

@franciszekjob franciszekjob Mar 16, 2026

Choose a reason for hiding this comment

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

note: Map contract does not have any constructor params, so passing them was incorrect. (similarly below)

@franciszekjob franciszekjob marked this pull request as ready for review March 16, 2026 12:48
@franciszekjob franciszekjob requested a review from a team as a code owner March 16, 2026 12:48
},
indoc! {r#"
Command: deploy
Error: Transaction execution error = TransactionExecutionErrorData { transaction_index: 0, execution_error: Message("Account transaction nonce is invalid.") }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update From<StarknetError> for SNCastStarknetError for more human-readable error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, theoretically we can but I guess that there are other similar cases in which we could match the data of tx execution error and map it to a different one. Can apply this, but ideally in separate PR. Is that ok @DelevoXDG ?

@franciszekjob franciszekjob requested a review from DelevoXDG March 16, 2026 16:36
Copy link
Collaborator

@ddoktorski ddoktorski left a comment

Choose a reason for hiding this comment

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

Do we have any tests that use fee arguments after this change?

@franciszekjob
Copy link
Contributor Author

franciszekjob commented Mar 18, 2026

Do we have any tests that use fee arguments after this change?

Yes, e.g. this one. And similarly for deploy, invoke

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants