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.

Sign up for GitHub

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

Jump to bottom

contract: return error rather than panic whenever possible #690

Merged
merged 11 commits into from
Jul 22, 2024
Merged

contract: return error rather than panic whenever possible #690

merged 11 commits into from
Jul 22, 2024

Conversation

ppca
Copy link
Contributor

@ppca ppca commented Jul 17, 2024 •
edited
Loading

all fn will return Result<_, MpcContractError>.

@ppca ppca marked this pull request as draft July 17, 2024 23:48
@ppca ppca marked this pull request as ready for review July 18, 2024 17:22
@ppca ppca linked an issue Jul 18, 2024 that may be closed by this pull request
Improve error handling in Contract, list all posible errors in enum or using [handle_result] #664
Closed
@ppca ppca force-pushed the xiangyi/replace_panic_str_w_err branch from 957a030 to 0c8f97d Compare July 18, 2024 23:41
ChaoticTempest
ChaoticTempest reviewed Jul 19, 2024
View reviewed changes
@frol
Copy link

frol commented Jul 19, 2024

@ppca @ChaoticTempest @volovyks Sorry for jumping in, but I'd like to invite you to participate in the Race of Sloths - a fun and gamified open-source contributions program. Consider mentioning @race-of-sloths user in your github comment or PR description to join!

See how the flow works here: near/nearcore#11778

ailisp
ailisp reviewed Jul 19, 2024
View reviewed changes
Copy link
Member

@ailisp ailisp left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes to the contract looks good overall, I didn't know about handle_result which will turn result into a failed action

ailisp
ailisp previously approved these changes Jul 19, 2024
View reviewed changes
volovyks
volovyks reviewed Jul 19, 2024
View reviewed changes
Copy link
Collaborator

@volovyks volovyks left a comment

Choose a reason for hiding this comment

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

Looks cool!

chain-signatures/contract/src/lib.rs Outdated
#[error("This sign request was removed from pending requests: timed out or completed.")]
RequestNotInPending,
#[error("Signature could not be verified.")]
SignatureNotVerified,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we please separate errors by function or scope where they can appear?
The end goal here is to give users a finite number of errors that can happen for each call they are making.

In this case, we have Timeout, which the user can get from sign() and SignatureNotVerified, which can only happen in respond.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good. Will do.

chain-signatures/contract/src/lib.rs
@@ -130,11 +187,14 @@ impl MpcContract {
}
}

fn remove_request(&mut self, request: SignatureRequest) {
fn remove_request(&mut self, request: SignatureRequest) -> Result<(), MpcContractError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to add a similar return Error type for sign()? This is the main function and it is a shame that it does not have strongly typed errors.

Copy link
Contributor Author

@ppca ppca Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

no I tried, it doesn't work. It will return () instead of the actual yield/resume result. Let me ask Saketh if it's possible figured out a way to do it. will change

@ppca ppca force-pushed the xiangyi/replace_panic_str_w_err branch from 0c8f97d to 0afe3e1 Compare July 19, 2024 23:52
@ppca
Copy link
Contributor Author

ppca commented Jul 19, 2024

using the trick of mimicking cross contract call to return a promise from the yield/resume

@volovyks @DavidM-D
Contract refactoring, API, README, etc (#706)
7c4ba38 
* fix contract tests, add attached and required to errors

* error and error messages naming

* return more specific errors in user facing functions

* fix error conversion issue

* separate user facing functions into one impl block

* move some of the contract structs to primitives

* API and envs README

* revert MpcError -> SignError

* fmt

* Update chain-signatures/contract/src/errors.rs

Co-authored-by: DavidM-D <[email protected]>

* Update chain-signatures/contract/src/errors.rs

Co-authored-by: DavidM-D <[email protected]>

* Update chain-signatures/contract/src/errors.rs

Co-authored-by: DavidM-D <[email protected]>

* Update chain-signatures/contract/src/errors.rs

Co-authored-by: DavidM-D <[email protected]>

* ignore RUSTSEC-2024-0357

* make sign_helper private

---------

Co-authored-by: DavidM-D <[email protected]>
ChaoticTempest
ChaoticTempest approved these changes Jul 22, 2024
View reviewed changes
Copy link
Member

@ChaoticTempest ChaoticTempest left a comment

Choose a reason for hiding this comment

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

not a huge fan of adding another promise on top just to have the return type, but we can probably fix this later without breaking anything since the return promise isn't directly used, just dropped to invoke the promise call

@volovyks volovyks merged commit 9db65d8 into develop Jul 22, 2024
4 checks passed
@volovyks volovyks deleted the xiangyi/replace_panic_str_w_err branch July 22, 2024 20:04
@github-actions GitHub Actions
Copy link

Terraform Feature Environment Destroy (dev-690)

Terraform Initialization ⚙️success

Terraform Destroy success

Show Destroy Plan 


No changes. No objects need to be destroyed.

Either you have not created any objects yet or the existing objects were
already deleted outside of Terraform.

Destroy complete! Resources: 0 destroyed.

Pusher: @volovyks, Action: pull_request, Working Directory: ``, Workflow: Terraform Feature Env (Destroy)

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

@ChaoticTempest ChaoticTempest ChaoticTempest approved these changes

@volovyks volovyks Awaiting requested review from volovyks

@ailisp ailisp Awaiting requested review from ailisp

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve error handling in Contract, list all posible errors in enum or using [handle_result]
5 participants
@ppca @frol @volovyks @ChaoticTempest @ailisp