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

Macros for testing contract standards #892

Open
Tarnadas opened this issue Aug 17, 2022 · 14 comments
Open

Macros for testing contract standards #892

Tarnadas opened this issue Aug 17, 2022 · 14 comments
Assignees

Comments

@Tarnadas
Copy link

In light of the recent NearX infinite mint exploit I’ve been thinking about how things like these could be circumvented.

I came up with the idea of having macros similar to the already existing contract standards, but they are used for testing the NEP functionalities. If an implementation does not use the contract standard macros for the implementation, then it should at least be possible to have a basic set of tests that need to pass.

For reference here’s the audit from NearX:
https://github.com/stader-labs/audits/blob/main/halborn/StaderLabs_NEARx_Smart_Contract_Security_Audit_Report_Halborn_Final.pdf
On page 17 the bug has been revealed, because sender and receiver are allowed to be the same. This bug could have been prevented, if a test exists for ft_transfer that checks exactly this.

@austinabell
Copy link
Contributor

To confirm, this is not a bug within any standard, correct? I checked and it seems we have this check. Is the goal of this to provide some high-level integration testing workflow to ensure a FT/NFT follows some general functionality, and check for cases like this?

@Tarnadas
Copy link
Author

No, if you use the macro from the standards implementation, you will have this check.

Exactly, I think there are some high level behaviors that can be checked like this. If someone implements his own FT/NFT, he could then load this macro to prevent such exploits in the future.

@austinabell
Copy link
Contributor

hmm, I don't think it would need to be a macro, I don't see any need for codegen, but this definitely seems useful. Unsure it would live within the SDK, though.

Guess it depends how it's expected to initialize the contract and what assumptions this library would provide. What would be the reason for this existing in the SDK/contract-standards?

@Tarnadas
Copy link
Author

I thought it would be quite similar to the existing macros that implement the core functionalities, which is why I think it belongs here.

Yes it might be hard to make assumptions about how the contract is initialized, which is why I think we need a macro so that the user can provide this information.
I've also seen these integration tests: https://github.com/near-examples/FT/blob/master/integration-tests/rs/src/tests.rs
I think they fit quite well, but would also lack a test that prevented the mentioned bug.

@Tarnadas
Copy link
Author

Also, I think the test macros or however it will be implemented must either be behind a feature flag to not add bloat to the sdk or move it entirely to another crate.

Now that I think about it, is it possible to add features only for dev-dependencies or would those features also be enabled in the regular dependencies?

@frol
Copy link
Collaborator

frol commented Jul 29, 2024

It would definitely be beneficial to have a function/set of functions that one could use in their contract tests like this:

#[test]
fn test_ft_core_interface() {
    let contract = MyContract::new();
    near_contract_standards::fungible_token::core::test(contract);
}

#[test]
fn test_ft_metadata_interface() {
    let contract = MyContract::new();
    near_contract_standards::fungible_token::metadata::test(contract);
}

...

There could be integration test helpers (using near-workspaces) and also unit test helpers (using the built-in unit-testing feature in near-sdk-rs)

There is no need for code-generation as test function can simply expect a type that implements the relevant trait:

fn test(contract: &mut impl near_contract_standards::fungible_token::core::FungibleTokenCore) {
}

@fabrobles92
Copy link

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

Hello Fab here, II am a full stack developer and Lately I've been involved contributing to an OSS project coded in Rust called Dojo Engine where I have contributed with Unit and integration tests using tokio::test.

Also as a Plus I am member of Web3 Community "Dojo Coding"

How I plan on tackling this issue

  • Join official channels of the project
  • Read the audit report to understand the exploit
  • Implement functions to use in contract tests that would check if they are vulnerable to the exploit

@frol
Copy link
Collaborator

frol commented Jul 29, 2024

@fabrobles92 Go ahead! Please, make sure you read my last comment for more details.

@fabrobles92
Copy link

@frol I have read the report and understood the bug, however in the report it says the bug has been solved by moving those methods from being public to be private.

Would you provide more context about the functions you want to implement and what they want to validate? And also where could be a good spot to add those helper functions?

@frol
Copy link
Collaborator

frol commented Jul 31, 2024

@fabrobles92 I'd like to have test functions next to trait interfaces that developers could use to test the expected outcomes based on the standard.

For example, here is the Fungible Token Core specification: https://nomicon.io/Standards/Tokens/FungibleToken/Core, and it states that there must be ft_transfer_call function that will transfer the tokens from the account that called the function to the receiver_id specified in the parameters, I'd like to see the following in near-sdk-contract-standards (pseudocode):

fn test(contract) {
    test_ft_transfer_call_ok(&contract);
    test_ft_transfer_call...
}

fn test_ft_transfer_call_ok(contract) {
    assert!(contract.get_balance("alice.near") == 1100);
    assert!(contract.get_balance("bob.near") == 0);
    let result = contract.tf_transfer_call("bob.near", ...);
    assert!(result == ...);
    assert!(contract.get_balance("alice.near") == 1000);
    assert!(contract.get_balance("bob.near") == 100);
}

Then, as a developer, I would be able to write the following contract: https://docs.rs/near-contract-standards/latest/near_contract_standards/fungible_token/core/trait.FungibleTokenCore.html#examples

use near_sdk::{near, PanicOnDefault, AccountId, PromiseOrValue};
use near_sdk::collections::LazyOption;
use near_sdk::json_types::U128;
use near_contract_standards::fungible_token::{FungibleToken, FungibleTokenCore};
use near_contract_standards::fungible_token::metadata::FungibleTokenMetadata;

#[near(contract_state)]
#[derive(PanicOnDefault)]
pub struct Contract {
    token: FungibleToken,
    metadata: LazyOption<FungibleTokenMetadata>,
}

#[near]
impl FungibleTokenCore for Contract {
    #[payable]
    fn ft_transfer(&mut self, receiver_id: AccountId, amount: U128, memo: Option<String>) {
        self.token.ft_transfer(receiver_id, amount, memo)
    }

    #[payable]
    fn ft_transfer_call(
        &mut self,
        receiver_id: AccountId,
        amount: U128,
        memo: Option<String>,
        msg: String,
    ) -> PromiseOrValue<U128> {
        self.token.ft_transfer_call(receiver_id, amount, memo, msg)
    }

    fn ft_total_supply(&self) -> U128 {
        self.token.ft_total_supply()
    }

    fn ft_balance_of(&self, account_id: AccountId) -> U128 {
        self.token.ft_balance_of(account_id)
    }
}

and test it easily:

#[cfg(all(test, not(target_arch = "wasm32")))]
mod tests {
    use super::*;

    #[test]
    fn test_fungible_token_core() {
        let contract = Contract::new_default_meta(accounts(1).into(), TOTAL_SUPPLY.into());
        near_contract_standards::fungible_token::core::test(contract);
    }

    #[test]
    fn test_fungible_token_metadata() {
        let contract = Contract::new_default_meta(accounts(1).into(), TOTAL_SUPPLY.into());
        near_contract_standards::fungible_token::metadata::test(contract);
    }
}

You can take inspiration for tests from fungible tokens contract example:

#[cfg(all(test, not(target_arch = "wasm32")))]
mod tests {
use near_contract_standards::fungible_token::Balance;
use near_sdk::test_utils::{accounts, VMContextBuilder};
use near_sdk::testing_env;
use super::*;
const TOTAL_SUPPLY: Balance = 1_000_000_000_000_000;
fn get_context(predecessor_account_id: AccountId) -> VMContextBuilder {
let mut builder = VMContextBuilder::new();
builder
.current_account_id(accounts(0))
.signer_account_id(predecessor_account_id.clone())
.predecessor_account_id(predecessor_account_id);
builder
}
#[test]
fn test_new() {
let mut context = get_context(accounts(1));
testing_env!(context.build());
let contract = Contract::new_default_meta(accounts(1).into(), TOTAL_SUPPLY.into());
testing_env!(context.is_view(true).build());
assert_eq!(contract.ft_total_supply().0, TOTAL_SUPPLY);
assert_eq!(contract.ft_balance_of(accounts(1)).0, TOTAL_SUPPLY);
}
#[test]
#[should_panic(expected = "The contract is not initialized")]
fn test_default() {
let context = get_context(accounts(1));
testing_env!(context.build());
let _contract = Contract::default();
}
#[test]
fn test_transfer() {
let mut context = get_context(accounts(2));
testing_env!(context.build());
let mut contract = Contract::new_default_meta(accounts(2).into(), TOTAL_SUPPLY.into());
testing_env!(context
.storage_usage(env::storage_usage())
.attached_deposit(contract.storage_balance_bounds().min.into())
.predecessor_account_id(accounts(1))
.build());
// Paying for account registration, aka storage deposit
contract.storage_deposit(None, None);
testing_env!(context
.storage_usage(env::storage_usage())
.attached_deposit(NearToken::from_yoctonear(1))
.predecessor_account_id(accounts(2))
.build());
let transfer_amount = TOTAL_SUPPLY / 3;
contract.ft_transfer(accounts(1), transfer_amount.into(), None);
testing_env!(context
.storage_usage(env::storage_usage())
.account_balance(env::account_balance())
.is_view(true)
.attached_deposit(NearToken::from_near(0))
.build());
assert_eq!(contract.ft_balance_of(accounts(2)).0, (TOTAL_SUPPLY - transfer_amount));
assert_eq!(contract.ft_balance_of(accounts(1)).0, transfer_amount);
}
}

@g4titanx
Copy link
Contributor

hey @frol i'd like to take this on

@dj8yfo
Copy link
Collaborator

dj8yfo commented Feb 25, 2025

@g4titanx i would debate that this would be much more useful if added to standard integration test helpers to near-workspaces with (maybe) adding a dependency from it to near-contract-standards (if needed).
And near-contract-standards could have example tests and description in README on how to do tests on Sandbox, then deploy to testnet and running similar tests with Testnet. Integration tests were mentioned in this issue but not quite highlighted they would be more informative than testing against a mock, which needs to be set up in some very specific way.

@g4titanx
Copy link
Contributor

good point! i agree that testing against actual contract behavior would be more informative than just mocking, so i'm thinking we could potentially do both:

  • add basic test helpers to near-contract-standards for unit testing core functionality
  • create more comprehensive integration test helpers in near-workspaces that could catch more complex interaction issues

what do you think?

@dj8yfo
Copy link
Collaborator

dj8yfo commented Feb 25, 2025

@g4titanx i think near-workspaces part should be prioritized. (https://github.com/near/near-sdk-rs/blob/0f46b0d596bbd5173b2c698b1fd7d6f8a2fc9a11/examples/fungible-token/tests/workspaces.rs can be looked at for ft for candidates of what can be made standard, though it requires deeper insight of the near-contracts-standards crate api surface).

Besides, the unit-testing part of near-sdk may have to be completely cut out in the future, though the probability of this scenario is currently perceived as low. So currently i'd not put any effort into it and bloat it at all or add something absolutely minimal, like 2 assertions for sanity check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: NEW❗
Development

No branches or pull requests

6 participants