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

Removes fee panics from sdk #1878

Merged
merged 3 commits into from
Sep 25, 2023
Merged

Removes fee panics from sdk #1878

merged 3 commits into from
Sep 25, 2023

Conversation

grarco
Copy link
Collaborator

@grarco grarco commented Sep 8, 2023

Describe your changes

Removes some wrong panic! that were mistakenly brought into the sdk by the gas&fee pr.

Indicate on which release or other PRs this topic is based on

v0.22.0

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@grarco grarco marked this pull request as ready for review September 8, 2023 11:19
@grarco grarco requested a review from mariari September 8, 2023 11:19
mariari
mariari previously approved these changes Sep 8, 2023
Copy link
Member

@mariari mariari left a comment

Choose a reason for hiding this comment

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

Overall looks reasonable.

Only suggestion I would have is maybe split the error.rs additions into their own commit so that there is a more clear point in where they are added

@@ -277,10 +277,11 @@ pub async fn aux_signing_data<
};

if fee_payer == masp_tx_key().to_public() {
panic!(
return Err(Error::Tx(TxError::Other(
Copy link
Member

Choose a reason for hiding this comment

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

For this one it may be god to just return a Error::Other. Though it doesn't matter too much.

grarco added a commit that referenced this pull request Sep 8, 2023
@grarco grarco force-pushed the grarco/remove-sdk-fee-panics branch from 6c5f80e to 1a61ae6 Compare September 8, 2023 15:44
@grarco grarco mentioned this pull request Sep 8, 2023
@grarco grarco force-pushed the grarco/remove-sdk-fee-panics branch from 1a61ae6 to 63467f7 Compare September 8, 2023 16:02
@adrianbrink adrianbrink self-requested a review September 11, 2023 07:42
grarco added a commit that referenced this pull request Sep 21, 2023
Fraccaman added a commit that referenced this pull request Sep 25, 2023
* origin/grarco/remove-sdk-fee-panics:
  changelog: add #1878
  Removes fee panics from sdk
  Adds fee-related errors to sdk
@brentstone brentstone merged commit 7a53e57 into main Sep 25, 2023
12 checks passed
@brentstone brentstone deleted the grarco/remove-sdk-fee-panics branch September 25, 2023 17:19
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.

4 participants