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

Contract refactoring, API, README, etc #706

Merged
merged 15 commits into from
Jul 22, 2024

Conversation

volovyks
Copy link
Collaborator

@volovyks volovyks commented Jul 21, 2024

  • fixed contract tests
  • renamed some errors and error messages (opinionated, open for a discussion about some of them)
  • moved user-facing functions into one impl block
  • added README with an API and a list of envs (mostly existing descriptions moved to a text file and formatted for better onboarding).

I've also tried to narrow down return error types in user-facing functions so they know what they can get exactly.

/// from
pub fn sign(&mut self, request: SignRequest) -> Result<near_sdk::Promise, MpcContractError>
/// to
pub fn sign(&mut self, request: SignRequest) -> Result<near_sdk::Promise, SignError>

But to do that we will need to get rid of the MpcContractError enum since it's implementation of

impl near_sdk::FunctionError for MpcContractError {
    fn panic(&self) -> ! {
        crate::env::panic_str(&self.to_string())
    }
}

conflicts with newly added impl for SignError, PublicKeyError, etc.
Let me know if you guys think this is an important change, or we can stick to MpcError everyvere.

@ppca based on your branch, since it has a lot of changes

@volovyks volovyks changed the title Serhii/contract fixes and api Contract refactoring, API, README, etc Jul 21, 2024
@volovyks volovyks requested review from ppca and ailisp July 21, 2024 11:04
@volovyks volovyks requested a review from ChaoticTempest July 21, 2024 11:59
@volovyks volovyks linked an issue Jul 21, 2024 that may be closed by this pull request
env!("CARGO_PKG_VERSION").to_string()
}

pub fn sign_helper(&mut self, request: SignatureRequest) {
Copy link

Choose a reason for hiding this comment

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

@volovyks should this be marked as [#private]?

otherwise, people could just call it jumping over sign (except if there is something about using the DATA_ID_REGISTER that I am not aware of)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed. Changed. cc @ppca

"{required_deposit}".to_string(),
return Err(MpcContractError::SignError(SignError::InsufficientDeposit(
deposit.as_yoctonear(),
required_deposit,
)));
}
Copy link

@gagdiez gagdiez Jul 22, 2024

Choose a reason for hiding this comment

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

@volovyks could we return the money if they exceed the payment? This would cost < 3Tgas, but help with the deposit estimate

if required > deposit {
  Promise::new(env::predecessor_account_id()).transfer(deposit - required_deposit);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, added to our roadmap: #708

@gagdiez
Copy link

gagdiez commented Jul 22, 2024

@volovyks the current cost is 1 yoctoNEAR when there are less than 4 requests, and then it ramps to 50milliNEAR * (1...4), which ranges from 0.05N to 0.20N... this might be a bit expensive.

Did we intended to use yoctoNear instead of milliNEAR?

match pending_requests {
    0..=CHEAP_REQUESTS => 1,
    _ => {
        (pending_requests - CHEAP_REQUESTS) as u128
            * NearToken::from_millinear(50).as_yoctonear()
    }
}

@volovyks volovyks mentioned this pull request Jul 22, 2024
@volovyks
Copy link
Collaborator Author

Audit is fixed in

@volovyks the current cost is 1 yoctoNEAR when there are less than 4 requests, and then it ramps to 50milliNEAR * (1...4), which ranges from 0.05N to 0.20N... this might be a bit expensive.

Did we intended to use yoctoNear instead of milliNEAR?

match pending_requests {
    0..=CHEAP_REQUESTS => 1,
    _ => {
        (pending_requests - CHEAP_REQUESTS) as u128
            * NearToken::from_millinear(50).as_yoctonear()
    }
}

We use yocto everywhere.
It may be expensive, but the intention is to prevent system overload. Attacks must be expensive in order to prevent them.

@ppca ppca merged commit 7c4ba38 into xiangyi/replace_panic_str_w_err Jul 22, 2024
4 checks passed
@ppca ppca deleted the serhii/contract-fixes-and-api branch July 22, 2024 18:54
Copy link

Terraform Feature Environment Destroy (dev-706)

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: @ppca, 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updated README
4 participants