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

[Feature] No access to accounts in preSetup #38

Closed
Tracked by #107
MarvinJanssen opened this issue Jun 9, 2021 · 11 comments
Closed
Tracked by #107

[Feature] No access to accounts in preSetup #38

MarvinJanssen opened this issue Jun 9, 2021 · 11 comments
Assignees

Comments

@MarvinJanssen
Copy link

MarvinJanssen commented Jun 9, 2021

Developers often need to do some initialising work in preSetup(), but they cannot access the Accounts map at this stage. It would be nice if you could.

@lgalabru lgalabru changed the title No access to accounts in preSetup [Feature] No access to accounts in preSetup Jun 9, 2021
@LNow
Copy link
Contributor

LNow commented Jun 9, 2021

@MarvinJanssen can you give an example of things you would like to do in preSetup()?

@MarvinJanssen
Copy link
Author

Deploy mock contracts or send initialising calls to contracts. Some of these things could be alleviated with deploy scripts but anything that is statically coded into the contract has to be present before Clarinet does its chain setup. Say that you are working on a prediction market that uses open-oracle, you'd want to mock that oracle in test.

@LNow
Copy link
Contributor

LNow commented Jun 10, 2021

What if we would split setup_chain into two separate phases?

  1. setup_accounts
  2. setup_contracts

If it would be possible, we could start writing like tests like this:

Clarinet.test('Super important test', (chain: Chain, accounts: Accounts) => {
  let mockOracle = new MockContract("imaginary-oracle-v1", "SP000000000000000000002Q6VF78");
	mockOracle.publicFn("price",{
    inputs: [typeDefs.ascii(24), typeDefs.ascii(24), typeDefs.uint()],
    returns: types.ok(types.uint(2000000))
  });
  
  // we could also have a addSetupTx(tx: Tx) function
  chain.addMock(mockOracle);

  const wallet_1 = accounts.get("wallet_1")!;
  
  let block = chain.mineBlock([
    Tx.contractCall(`SP000000000000000000002Q6VF78.imaginary-oracle-v1`, "price",
    [
        types.ascii("usd"),
        types.ascii("stx"),
        types.uint(5)
    ], wallet_1.address),
  ]);

  console.info(block)
})

As long as we won't call mineBlock, mineEmptyBlock, mineEmptyBlockUntil, callReadOnlyFn, getAssetsMaps only accounts would be set up.
Once we call any of these functions, then all our contracts as well as mocks will be deployed and function we call will be executed.

I think this should give us a bit more pleasant so called developer experience.

Clarinet code would look then more or less like this:

export interface Accounts extends Map<string, Account> {}

export class Clarinet {
  public static test(name: string, fn: (chain:Chain, accounts: Accounts)=> void){  
    let chain = new Chain(name);
    let accounts = chain.accounts;

    const testDefinition: Deno.TestDefinition = {
      name: name,
      fn: async () => {
        await fn(chain, accounts);
      }
    }

    Deno.test(testDefinition);
  }
}

export class Chain {
  sessionId: number;
  accounts: Accounts = new Map();
  private isReady: boolean = false;
  private transactions: Array<Tx> = [];
  private name: string;

  constructor(name: string) {
    this.name = name;

    (Deno as any).core.ops();
    let result = (Deno as any).core.jsonOpSync("setup_account");

    this.sessionId = result["session_id"]

    for (let account of result["accounts"]) {
      this.accounts.set(account.name, account);
    }
  }

  private setUp() {
    let result = (Deno as any).core.jsonOpSync("setup_contracts", {
      name: this.name,
      transactions: this.transactions
    })
    // we reset accounts as their balances might be different after this phase
    this.accounts.clear();
    for (let account of result["accounts"]) {
      this.accounts.set(account.name, account);
    }

    this.isReady = true;
  }

  addMock(mock: MockContract):void {
    this.transactions.push(Tx.deployContract(mock));
  }

  mineBlock(transactions: Array<Tx>) {
    if(!this.isReady) {
      this.setUp();
    }

    let result = (Deno as any).core.jsonOpSync("mine_block", {
      sessionId: this.sessionId,
      transactions: transactions,
    });
    let block: Block = {
      height: result.block_height,
      receipts: result.receipts,
    };
    return block;
  }
....
....
}

@MarvinJanssen
Copy link
Author

Interesting idea, although I wonder if this would be clear to people. I can see that people will try to deploy mocks after calling those functions.

If one can only deploy mocks during preSetup(), then there is a clear division. (Although I wonder if there would be a valid reason for being able to change the output / side-effects of a mock during the test() phase? If so, then then these could also be passed to test(), see #40. )

@LNow
Copy link
Contributor

LNow commented Jun 10, 2021

The thing is that if your contract somehow depends on a mock, you can't deploy it after. It must be deployed before you can any function in you contract. Otherwise you won't be able to deploy it.

Deploying mock is nothing else but part of Arrange phase of your test.
With current test function signature you have something like this:

Clarinet.test({
  name: 'name of your test',
  preSetup(): Tx[] {
    //ARRANGE 
    // some setup transactions
  },
  async fn(chain: Chain, accounts: Map<string, Account>) {
    //ARRANGE - continuation
    
    //ACT

   //ASSERT
  }
});

And I think this one is a bit cleaner (only one arrange section):

Clarinet.test('name of your test', async (chain: Chain, accounts: Accounts) => {
   // ARRANGE

   // ACT

   // ASSERT
});

Changing mock behavior in the middle of a test? To me it screams for a separate test.
If there would be a way to intercept contract-calls sent to a mock, pass them to JS that would calculate valid response that would be a game changer :)
Baby steps - we'll get there.

@MarvinJanssen
Copy link
Author

Point is that people might not be aware of the order of operations and try to deploy a mock after calling mineBlock() by accident. That's why I like the readability of having a separate preSetup(). (Which we should arguably just call setup().) It also makes it easier to define a few setups that can be reused for different tests.

Changing mock behavior in the middle of a test? To me it screams for a separate test.

Indeed, which is why I wondered if there is a valid use-case at all.

If there would be a way to intercept contract-calls sent to a mock, pass them to JS that would calculate valid response that would be a game changer :)

Yep, I explored this but it leaves things to be desired. You'd need to do some interception and have it call back into a callback defined in test. Sounds tricky. It is why my MockContract sample generates all that is needed in JS, keeping Clarinet light. I think we can get away with setting a static return value from a mock call. I'm not sure if there are real use-cases for requiring calculation. (Do you have any examples?) The only thing we might want is a way to trigger a side-effect, like updating a map or data var in the mock contract to be used by a subsequent tx in the same test, or triggering a panic or event. But one can again wonder if setting static return values is enough. As any data to be returned from a public function can likewise be mocked.

@lgalabru lgalabru mentioned this issue Aug 27, 2021
10 tasks
@lgalabru lgalabru added the 1.0 label Aug 27, 2021
@lgalabru
Copy link
Contributor

lgalabru commented Dec 6, 2021

I'm trying to prioritize, is this feature request solving actual pain points? cc @MarvinJanssen @LNow

@lgalabru lgalabru added the S label Jan 12, 2022
@lgalabru
Copy link
Contributor

lgalabru commented Feb 8, 2022

I came up with a solution for this with #240, an example is described here: friedger/clarity-catamaranswaps#2.
Would love to get your thoughts on this approach @MarvinJanssen and @LNow!

@lgalabru lgalabru self-assigned this Feb 8, 2022
@LNow
Copy link
Contributor

LNow commented Feb 9, 2022

@lgalabru this look very nice.
Would it be possible to add "super advanced" version of it, let's say manualSetup in which users would have the possibility to manually control order of deployed contracts and block-height at which they are deployed?

manualSetup: async (chain: Chain, accounts: Map<string, Account>, contracts: Map<string, Contract>) => {
    const deployer = accounts.get("deployer")!;
   chain.mineEmptyBlock(400);

    chain.mineBlock([
      Tx.deployContract("cool-contract", contracts.get("cool-contract")!.code, deployer.address)
    ]);
 
    chain.mineBlock([
      Tx.contractCall("cool-contract", "init", [], deployer.address)
    ]);
    chain.mineEmptyBlock(10);

   chain.mineBlock([
      Tx.deployContract("another-contract", contracts.get("another-contract")!.code, deployer.address)
    ]);
  },

It would help simulate more realistic deployments.

@unclemantis
Copy link

unclemantis commented Feb 9, 2022 via email

@lgalabru
Copy link
Contributor

Addressed in v0.28.0

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

No branches or pull requests

4 participants