Update tokenURI tests

[andreivladbrg]

Closes #1071

[smol-ninja]

Can you please give a tl'dr of what you changed?

• In the issue description, it is mentioned that "The idea is to no longer skip these tests after we finish all changes related to 2.0.0" but I do not understand how you fixed that in this PR?

• Also what is that address that you used 0x923b5Ab3714FD343316aF5A5434582Fd16722523?

• Given that we only have one contract now, can we have just one contract as TokenURI_Lockup_Integration_Concrete_Test instead of three?

[andreivladbrg]

Can you please give a tl'dr of what you changed?

changed 1. the lockup address (which is included in the SVG) and 2. the strings hardcoded in expectedTokenURI var tests.

The idea is to no longer skip these tests after we finish all changes related to 2.0.0"

are you familiar with the skipOnMismatch modifier? the idea is to not update the string everytime we make a change (a new change in the lockup contract would produce a different address) as it is not that friendly to change a big string, so we added this modifier

v2-core/tests/integration/concrete/lockup-linear/token-uri/tokenURI.t.sol

Lines 22 to 29 in edb95df

	/// @dev To make these tests noninvasive, they are run only when the contract address matches the hard coded value.
	modifier skipOnMismatch() {
	if (address(lockup) == LOCKUP) {
	_;
	} else {
	console2.log(StdStyle.yellow('Warning: "LockupLinear.tokenURI" tests skipped due to address mismatch'));
	}
	}

Also what is that address that you used 0x923b5Ab3714FD343316aF5A5434582Fd16722523?

the address of the lockup contract. it is just a method to run the tests only when a certain criteria is met

Given that we only have one contract now, can we have just one contract as TokenURI_Lockup_Integration_Concrete_Test instead of three?

good question. we can indeed keep only one contract. hmm, which makes me think, why not mot modify the NFTDescriptor to include the lockupModel, wdyt?

[smol-ninja]

we can indeed keep only one contract. hmm, which makes me think, why not mot modify the NFTDescriptor to include the lockupModel

IMO we can but it will just be an additional information. What Traders / Users need to know is already a part of the SVG. I think the end users do not care about the specific stream model but only the shape which can only be visualised on the UI.

Regarding your other points, I have sent you a msg on Slack for some clarification.

[andreivladbrg]

fair enough, let's keep the current version

[smol-ninja]

let's keep the current version

What about DRY'ify it with a single test contract TokenURI_Lockup_Integration_Concrete_Test?

[andreivladbrg]

What about DRY'ify it with a single test contract TokenURI_Lockup_Integration_Concrete_Test?

yeah, i wasn’t referring to the tests but to the NFTDescriptor contract (this was the subject of the matter).

as for the tests, i mentioned in my initial response, we can have one contract

[smol-ninja]

I used https://www.diffchecker.com/ to compare the file. Two comments below.

[PaulRBerg]

While we are at this, we should remove the payment qualifier in payment stream to avoid any confusion with the UI (the UI now refers to Flow streams as 'payment streams'). So we should say just 'stream'.

[smol-ninja]

As decided on Slack, @andreivladbrg can you please include the changes that remove lines of code related to the backward compatibility? We can also delete this fork test from running on previous versions.

[smol-ninja]

Closing it in favour of #1113.