chore: nonfungible example contract tests with `pop-drink` #565

chungquantin · 2025-05-12T15:59:13Z

The PR add drink tests to nonfungible example contract.

codecov-commenter · 2025-05-12T16:41:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.47%. Comparing base (e356fb4) to head (13d33dd).

@@           Coverage Diff           @@
##             main     #565   +/-   ##
=======================================
  Coverage   63.47%   63.47%           
=======================================
  Files         121      121           
  Lines       23838    23838           
  Branches    23838    23838           
=======================================
  Hits        15130    15130           
  Misses       7664     7664           
  Partials     1044     1044

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

evilrobot-01 · 2025-05-14T13:14:52Z

Please target this to the fungibles PR branch, so that we dont have to review the same changes which appear in that PR again here.

pop-api/examples/nonfungibles/lib.rs

chungquantin · 2025-05-15T04:05:28Z

pop-api/examples/nonfungibles/lib.rs

 		}

 		/// Returns the owner of an item, if any.
 		#[ink(message)]
-		pub fn owner_of(&self, item: ItemId) -> Result<Option<AccountId>> {
-			api::owner_of(self.id, item).map_err(Psp34Error::from)
+		pub fn owner_of(&self, item: ItemId) -> Option<AccountId> {


To keep it consistent with the fungibles example

evilrobot-01 · 2025-05-16T06:18:22Z

@chungquantin please can you kindly resolve the merge conflicts?

* fix visibility for pallet_nfts types used as call arguments Changes from the commit: paritytech/polkadot-sdk#3634 * [pallet-nfts, pallet_uniques] - Expose private structs Changes made from upstream: paritytech/polkadot-sdk#6087 * Adds BlockNumberProvider in multisig, proxy and nft pallets Upstream commit: paritytech/polkadot-sdk#5723 * Removed unused dependencies (partial progress) Upstream commit: paritytech/polkadot-sdk#7329 * implement DecodeWithMemTracking for frame pallets paritytech/polkadot-sdk#7598 * chore: bump pallet-nfts version to 34.1.0 * feat: adds BlockNumberProvider in nonfungbiles pallet * chore: update lock file of pop-api to sync nfts * chore: BlockNumberProvider for devnet nonfungibles * chore: fix benchmarking to use new BlockNumberFor * chore: revert changes * fix: missing DecodeWithMemTracking * chore: update weights of pallets * refactor(nonfungibles): remove unused BlockNumberProvider type * chore: remove unused Zero trait * chore: revert non-relevant changes * chore: revert non-relevant changes This reverts commit 182486a. * chore: benchmarking replace with BlockNumberProvider * chore(pallet-nfts): update weights

* chore(api/examples/fungible): bump drink version * chore(ci): test contract in examples * chore: update drink dependency branch * chore: resolve review comments

evilrobot-01 · 2025-05-16T06:52:20Z

pop-api/examples/nonfungibles/Cargo.toml

@@ -10,6 +10,12 @@ pop-api = { path = "../../../pop-api", default-features = false, features = [
 	"nonfungibles",
 ] }

+[dev-dependencies]
+drink = { package = "pop-drink", git = "https://github.com/r0gue-io/pop-drink", branch = "chungquantin/feat-nfts", features = [ "devnet" ] }


This should be merged to main and updated here before being merged here.

PR approved, so once merged this can be updated.

pop-api/examples/nonfungibles/lib.rs

evilrobot-01 · 2025-05-16T07:01:02Z

pop-api/examples/nonfungibles/tests.rs

+// The contract bundle provider.
+//
+// See https://github.com/r0gue-io/pop-drink/blob/main/crates/drink/drink/test-macro/src/lib.rs for more information.
+#[drink::contract_bundle_provider]


Can this definition be moved within the deploy helper function below? It eliminates the complexity/noise of having yet another thing to define, where someone can just copy the deploy function in its entirey and get this definition included.

The separate usage on 77 should then be using the deploy helper.

Good idea! I have moved it to the deploy method in 7607a1a

pop-api/examples/nonfungibles/tests.rs

evilrobot-01 · 2025-05-16T07:04:08Z

pop-api/examples/nonfungibles/tests.rs

+
+// Deployment and constructor method tests.
+
+fn deploy_with_default(session: &mut Session<Pop>) -> Result<AccountId> {


Would move all the helpers to the bottom after tests. I see value in firstly showing how the sandbox is defined/initialised, but then it should be showing the tests and not more boiler plate.

Move this method into Contract struct and rename it to default. Hence, Contract::default() means deploying a contract by default to keep it short.

evilrobot-01 · 2025-05-16T07:07:34Z

pop-api/examples/nonfungibles/tests.rs

+
+// A set of helper methods to test the contract calls.
+
+fn collection_id(session: &mut Session<Pop>) -> CollectionId {


Would suggest a pattern like

pop-node/pop-api/integration-tests/src/messaging.rs

Lines 402 to 485 in 9cd371f

struct Contract {

address: AccountId32,

id: AccountId32,

}

impl Contract {

fn new() -> Self {

let address = instantiate(CONTRACT, INIT_VALUE, vec![]);

Self { address: address.clone(), id: address }

}

fn ismp_get(

&self,

id: MessageId,

request: Get,

fee: Balance,

callback: bool,

) -> Result<(), Error> {

let result = self.call("ismp_get", (id, request, fee, callback).encode(), 0);

<Result<(), Error>>::decode(&mut &result.data[1..])

.unwrap_or_else(|_| panic!("Contract reverted: {:?}", result))

}

fn ismp_post(

&self,

id: MessageId,

request: Post,

fee: Balance,

callback: bool,

) -> Result<(), Error> {

let result = self.call("ismp_post", (id, request, fee, callback).encode(), 0);

<Result<(), Error>>::decode(&mut &result.data[1..])

.unwrap_or_else(|_| panic!("Contract reverted: {:?}", result))

}

fn xcm_new_query(

&self,

id: MessageId,

responder: Location,

timeout: BlockNumber,

callback: bool,

) -> Result<Option<QueryId>, Error> {

let result = self.call("xcm_new_query", (id, responder, timeout, callback).encode(), 0);

<Result<Option<QueryId>, Error>>::decode(&mut &result.data[1..])

.unwrap_or_else(|_| panic!("Contract reverted: {:?}", result))

}

fn poll(&self, request: MessageId) -> Result<Option<Status>, Error> {

let result = self.call("poll", request.encode(), 0);

Result::<Option<Status>, Error>::decode(&mut &result.data[1..])

.unwrap_or_else(|_| panic!("Contract reverted: {:?}", result))

}

fn get(&self, request: MessageId) -> Result<Option<Vec<u8>>, Error> {

let result = self.call("get", request.encode(), 0);

Result::<Option<Vec<u8>>, Error>::decode(&mut &result.data[1..])

.unwrap_or_else(|_| panic!("Contract reverted: {:?}", result))

}

fn remove(&self, request: MessageId) -> Result<(), Error> {

let result = self.call("remove", request.encode(), 0);

<Result<(), Error>>::decode(&mut &result.data[1..])

.unwrap_or_else(|_| panic!("Contract reverted: {:?}", result))

}

fn call(&self, function: &str, params: Vec<u8>, value: u128) -> ExecReturnValue {

let function = function_selector(function);

let params = [function, params].concat();

bare_call(self.address.clone(), params, value).expect("should work")

}

fn last_event(&self) -> Vec<u8> {

let events = System::read_events_for_pallet::<pallet_contracts::Event<Runtime>>();

let contract_events = events

.iter()

.filter_map(|event| match event {

pallet_contracts::Event::<Runtime>::ContractEmitted { contract, data, .. }

if contract == &self.address =>

Some(data.as_slice()),

_ => None,

})

.collect::<Vec<&[u8]>>();

contract_events.last().unwrap().to_vec()

}

}

instead, so it then becomes obvious in the tests that one is calling a function on a contract via contract.collection_id

Thanks for the suggestion, I have refactored to bring all helpers method under the Contract struct in 3dd2ad4

evilrobot-01 · 2025-05-19T14:21:25Z

@chungquantin Please could you kindly rebase again to address the CI issues and then once all green I can review again.

evilrobot-01

A few improvement suggestions, along with identification of a few possible flaws.

evilrobot-01 · 2025-05-20T08:52:57Z

pop-api/examples/nonfungibles/Cargo.toml

@@ -10,6 +10,12 @@ pop-api = { path = "../../../pop-api", default-features = false, features = [
 	"nonfungibles",
 ] }

+[dev-dependencies]
+drink = { package = "pop-drink", git = "https://github.com/r0gue-io/pop-drink", branch = "chungquantin/feat-nfts", features = [ "devnet" ] }


PR approved, so once merged this can be updated.

pop-api/examples/nonfungibles/lib.rs

pop-api/examples/nonfungibles/tests.rs

evilrobot-01 · 2025-05-20T08:57:17Z

pop-api/examples/nonfungibles/tests.rs

+fn new_constructor_works(mut session: Session) {
+	let _ = env_logger::try_init();
+	// Deploys a new contract.
+	let contract = Contract::default(&mut session).unwrap();


Suggested change

let contract = Contract::default(&mut session).unwrap();

let contract = Contract::new(&mut session, None).unwrap();

Why not new, deploy, or instantiate? Default doesnt imply a deployment action to me, but rather returning a default value of the Contract struct. I get it is a helper, but it doesnt convey the right intent imo.

Would just use the new() function with None instead, only as that most closely matches the contract constructor definition, which is the goal with the Contract struct - a type-safe wrapper for invoking calls on the contract.

Thanks. I have removed the method default, agree that it makes the code readers confusing if we deploy inside the method. I changed it to new as you suggested. Resolved in 3e1b812

evilrobot-01 · 2025-05-20T08:59:21Z

pop-api/examples/nonfungibles/tests.rs

+	// Deploys a new contract.
+	let contract = Contract::default(&mut session).unwrap();
+	// Collection exists after the deployment.
+	assert_eq!(session.sandbox().collection_owner(&COLLECTION), Some(contract.address.clone()));


Shouldnt need to clone, can probably use .collection_owner(&COLLECTION).as_ref()

Resolved in 3e1b812

evilrobot-01 · 2025-05-21T07:23:20Z

pop-api/examples/nonfungibles/tests.rs

+		format!("{:?}", DestroyWitness { item_metadatas: 0, item_configs: 0, attributes: 0 });
+
+	// Error returned "Not enough data to fill buffer" due to contract termination.
+	assert!(session.call::<String, ()>("destroy", &[witness_string], None).is_err());


This could be improved imo. it seems to stem from the fact that the call impl in pop-drink panics, eliminating the possibility of handling this cleanly. This means that every contract will have this messy approach to termination.

The "Not enough data to fill buffer" should not be leaked out beyond pop-drink - perhaps it can be converted to a known error type?

Resolved in dd7ca2e. I investigated and seems like the data returned is empty if the contract is terminated: https://github.com/use-ink/ink/blob/master/integration-tests/public/contract-terminate/lib.rs#L81 which also needs this PR in drink to be merged to support that: r0gue-io/pop-drink#35

evilrobot-01 · 2025-05-21T07:24:52Z

pop-api/examples/nonfungibles/tests.rs

+// A set of helper methods to test the contract deployment and calls.
+
+// Deploys the contract with `NO_SALT and `INIT_VALUE`.
+fn deploy(


This seems to only be used within Contract::new() now, so why not eliminate this function and just define it there instead?

Agree, I have moved its logic to the new method: 3e1b812

evilrobot-01 · 2025-05-21T07:25:55Z

pop-api/examples/nonfungibles/tests.rs

+		Ok(Self { address: contract })
+	}
+
+	fn default(session: &mut Session<Pop>) -> Result<Self> {


Would eliminate and just use new(session, None) personally. Its a few extra characters but conveys intent better.

Agree, I have removed the method in 3e1b812

evilrobot-01 · 2025-05-21T07:26:22Z

pop-api/examples/nonfungibles/tests.rs

+	// Deploy a new contract.
+	fn new(session: &mut Session<Pop>, max_supply: Option<u32>) -> Result<Self> {
+		let contract = deploy(session, "new", vec![format!("{:?}", max_supply)], NO_SALT)?;
+		Ok(Self { address: contract })


Address is never used, which indicates a flaw.

With 76080ba that refactored to use call_with_address, this is now valid.

evilrobot-01 · 2025-05-21T07:28:41Z

pop-api/examples/nonfungibles/tests.rs

+	}
+
+	fn collection_id(&self, session: &mut Session<Pop>) -> CollectionId {
+		call::<Pop, CollectionId, Psp34Error>(session, "collection_id", vec![], None).unwrap()


None of these functions use the contract address, indicating that the pop-drink::call function is flawed in a multi-contract setting. See https://github.com/use-ink/drink/blob/94835ac3124cc02d556be1d9ff9fdf419fc24465/examples/quick-start-with-drink/lib.rs#L169

My assumption is that currently if I deploy two instances via Contract::new(), creating two collections, I will only ever be able to operate on the first collection despite calling methods on the second contract instance.

Resolved in r0gue-io/pop-drink#35 and 76080ba with new call_with_address added.

* ci: build runtimes using production profile * docs(ci): Explain the usage of production profile

* chore: sync with polkadot-stable2503-1 * fix: integration-tests as per sdk#7913 * fix(integration-tests): accomodate for different xcm weights between runtime * chore(pop-api): update Cargo.lock

* chore: depend on polkadot-stable2503-4 github tag * chore: revert subxt version * chore: update sc-network dependency * chore(pop-api): update Cargo.lock

* chore(testnet): bump versions to v0.5.3 * chore: point drink to stable2503-4

chungquantin added 4 commits May 8, 2025 14:36

chore(api/examples/fungible): bump drink version

c90ee6d

chore(ci): test contract in examples

ccc6d98

chore: update drink dependency branch

80eaa45

feat: init test file

9adc94a

chungquantin self-assigned this May 12, 2025

chungquantin added 4 commits May 13, 2025 12:39

feat: add tests and helper methods

b1d40bf

feat(api+drink): derive Debug type and add burn, destroy tests

dd42f4c

feat: finalize the tests

5f601cd

chore: expose NftsError to public

c0797ee

chungquantin marked this pull request as ready for review May 13, 2025 13:40

chungquantin requested review from Daanvdplas and a team May 13, 2025 14:48

chungquantin changed the base branch from main to chungquantin/api/fungible/chore-bump_example_drink May 15, 2025 04:04

chungquantin commented May 15, 2025

View reviewed changes

pop-api/examples/nonfungibles/lib.rs Outdated Show resolved Hide resolved

chungquantin commented May 15, 2025

View reviewed changes

chore: update comments

a88f9ad

Base automatically changed from chungquantin/api/fungible/chore-bump_example_drink to main May 16, 2025 06:16

chungquantin added 4 commits May 16, 2025 13:19

chore(api/examples/fungible): bump drink version (#562)

6b6884f

* chore(api/examples/fungible): bump drink version * chore(ci): test contract in examples * chore: update drink dependency branch * chore: resolve review comments

Merge branch 'main' into chungquantin/chore-nonfungible_tests

f4688de

chore: remove unrelevant changes

5d1e587

evilrobot-01 reviewed May 16, 2025

View reviewed changes

chungquantin added 3 commits May 19, 2025 09:40

refactor(api/examples/nonfungibles): Contract struct

3dd2ad4

fix(api/examples/nonfungibles): revert destroy

7607a1a

fix(api/examples/nonfungibles): revert destroy

3afb908

Merge branch 'main' into chungquantin/chore-nonfungible_tests

712a3a9

chungquantin requested a review from evilrobot-01 May 19, 2025 23:31

evilrobot-01 reviewed May 21, 2025

View reviewed changes

chungquantin added 2 commits May 22, 2025 13:31

chore: resolve review comments

3e1b812

refactor: call_with_address

20af3a3

chungquantin force-pushed the chungquantin/chore-nonfungible_tests branch from 76080ba to 20af3a3 Compare May 22, 2025 07:56

al3mart and others added 9 commits May 22, 2025 19:33

fix(ci): pin stable-1.86.0 version (#570)

6a484d5

refactor(ci): build runtimes using production profile (#567)

fc71d84

* ci: build runtimes using production profile * docs(ci): Explain the usage of production profile

chore: sync with polkadot-stable2503-1 (#559)

682eb5d

* chore: sync with polkadot-stable2503-1 * fix: integration-tests as per sdk#7913 * fix(integration-tests): accomodate for different xcm weights between runtime * chore(pop-api): update Cargo.lock

chore: sync with polkadot-stable2503-4 (#571)

2256c9d

* chore: depend on polkadot-stable2503-4 github tag * chore: revert subxt version * chore: update sc-network dependency * chore(pop-api): update Cargo.lock

chore: pop-runtime-testnet v0.5.3 (#569)

2c21b2d

* chore(testnet): bump versions to v0.5.3 * chore: point drink to stable2503-4

chore(api/examples/fungible): bump drink version

b3b9623

chore: bump cargo.lock

00829c0

chore: update the SessionError

dd7ca2e

Merge branch 'main' into chungquantin/chore-nonfungible_tests

13d33dd

chungquantin requested a review from evilrobot-01 May 22, 2025 15:23


		// Deployment and constructor method tests.

		fn deploy_with_default(session: &mut Session<Pop>) -> Result<AccountId> {


		// A set of helper methods to test the contract calls.

		fn collection_id(session: &mut Session<Pop>) -> CollectionId {

	struct Contract {
	address: AccountId32,
	id: AccountId32,
	}
	impl Contract {
	fn new() -> Self {
	let address = instantiate(CONTRACT, INIT_VALUE, vec![]);
	Self { address: address.clone(), id: address }
	}

	fn ismp_get(
	&self,
	id: MessageId,
	request: Get,
	fee: Balance,
	callback: bool,
	) -> Result<(), Error> {
	let result = self.call("ismp_get", (id, request, fee, callback).encode(), 0);
	<Result<(), Error>>::decode(&mut &result.data[1..])
	.unwrap_or_else(\|_\| panic!("Contract reverted: {:?}", result))
	}

	fn ismp_post(
	&self,
	id: MessageId,
	request: Post,
	fee: Balance,
	callback: bool,
	) -> Result<(), Error> {
	let result = self.call("ismp_post", (id, request, fee, callback).encode(), 0);
	<Result<(), Error>>::decode(&mut &result.data[1..])
	.unwrap_or_else(\|_\| panic!("Contract reverted: {:?}", result))
	}

	fn xcm_new_query(
	&self,
	id: MessageId,
	responder: Location,
	timeout: BlockNumber,
	callback: bool,
	) -> Result<Option<QueryId>, Error> {
	let result = self.call("xcm_new_query", (id, responder, timeout, callback).encode(), 0);
	<Result<Option<QueryId>, Error>>::decode(&mut &result.data[1..])
	.unwrap_or_else(\|_\| panic!("Contract reverted: {:?}", result))
	}

	fn poll(&self, request: MessageId) -> Result<Option<Status>, Error> {
	let result = self.call("poll", request.encode(), 0);
	Result::<Option<Status>, Error>::decode(&mut &result.data[1..])
	.unwrap_or_else(\|_\| panic!("Contract reverted: {:?}", result))
	}

	fn get(&self, request: MessageId) -> Result<Option<Vec<u8>>, Error> {
	let result = self.call("get", request.encode(), 0);
	Result::<Option<Vec<u8>>, Error>::decode(&mut &result.data[1..])
	.unwrap_or_else(\|_\| panic!("Contract reverted: {:?}", result))
	}

	fn remove(&self, request: MessageId) -> Result<(), Error> {
	let result = self.call("remove", request.encode(), 0);
	<Result<(), Error>>::decode(&mut &result.data[1..])
	.unwrap_or_else(\|_\| panic!("Contract reverted: {:?}", result))
	}

	fn call(&self, function: &str, params: Vec<u8>, value: u128) -> ExecReturnValue {
	let function = function_selector(function);
	let params = [function, params].concat();
	bare_call(self.address.clone(), params, value).expect("should work")
	}

	fn last_event(&self) -> Vec<u8> {
	let events = System::read_events_for_pallet::<pallet_contracts::Event<Runtime>>();
	let contract_events = events
	.iter()
	.filter_map(\|event\| match event {
	pallet_contracts::Event::<Runtime>::ContractEmitted { contract, data, .. }
	if contract == &self.address =>
	Some(data.as_slice()),
	_ => None,
	})
	.collect::<Vec<&[u8]>>();
	contract_events.last().unwrap().to_vec()
	}
	}

	let contract = Contract::default(&mut session).unwrap();
	let contract = Contract::new(&mut session, None).unwrap();

chore: nonfungible example contract tests with pop-drink #565

Are you sure you want to change the base?

chore: nonfungible example contract tests with pop-drink #565

Uh oh!

Conversation

chungquantin commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

evilrobot-01 commented May 14, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evilrobot-01 commented May 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evilrobot-01 commented May 19, 2025

Uh oh!

evilrobot-01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chungquantin May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chungquantin May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chungquantin May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chungquantin May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

chore: nonfungible example contract tests with `pop-drink` #565

chore: nonfungible example contract tests with `pop-drink` #565

chungquantin commented May 12, 2025 •

edited

Loading

codecov-commenter commented May 12, 2025 •

edited

Loading

chungquantin May 22, 2025 •

edited

Loading

chungquantin May 22, 2025 •

edited

Loading

chungquantin May 22, 2025 •

edited

Loading

chungquantin May 22, 2025 •

edited

Loading