search

tezos-commons / baseDAO

BaseDAO - a generic smart contract framework for DAOs on Tezos
57 stars 15 forks source link

Update morley dependencies and update CI to use kathmandu chains #341

Closed sras closed 2 years ago

sras commented 2 years ago

Description

Update deps of Morley and friends and include Kathmandu network in CI tests

Related issue(s)

Resolves #

:white_check_mark: Checklist for your Pull Request

Related changes (conditional)

Stylistic guide (mandatory)

lierdakil commented 2 years ago

Also, please update morley-infra, poor CI has been running since yesterday in a loop stuck with "unknown protocol".

lierdakil commented 2 years ago

Certainly it's not partial pattern matches alone, but I don't know enough compiler internals to really say what else is at play. I vaguely suspect polymorphic code might have exponentially increased the amount of work the desugarer has to do wrt partial pattern matches or vice versa, but I might be totally wrong, and I couldn't tell you the exact details even if I'm right. I'm pretty convinced this is an unfortunate interaction of multiple subsystems. Staring at the core output for a few hours might offer more insight, but that's not something I'm very eager to do, core is very verbose, and finding relevant tidbits takes a lot of effort.

пн, 17 окт. 2022 г., 02:29 Sandeep.C.R @.***>:

@.**** commented on this pull request.

In haskell/test/Test/Ligo/BaseDAO/OffChainViews.hs https://github.com/tezos-commons/baseDAO/pull/341#discussion_r996524528:

-mkFA2Tests storage mc = testGroup "FA2 off-chain views"

  • [ testGroup "governance_token" $
  • [ testCase "Get the address and token_id of the associated FA2 contract" $
  • runView @GovernanceToken (governanceTokenView mc) storage NoParam
  • @?= (Right $ GovernanceToken genesisAddress FA2.theTokenId)
  • ]
  • ] +mkFA2Tests = testGroup "FA2 off-chain views" [] +-- TODO: Uncomment when some way to run views is available. +-- [ testGroup "governance_token" $ +-- [ testCase "Get the address and token_id of the associated FA2 contract" $ +-- runView @GovernanceToken (governanceTokenView mc) storage NoParam +-- @?= (Right $ GovernanceToken (MkAddress genesisAddress) FA2.theTokenId) +-- ] +-- ]

The compile time has dropped from 30 minutes to 2 minutes 40 secs for a recompiling to account for change in a single module. I cannot figure how this slowdown could be caused by partial pattern matches alone. Is it possible that the constraints somehow play a role in this? becuase IIRC introducing them triggered the slow down, when the partial patterns were there from the begining ..

Anyway this change takes away some massive pain, so a big thanks for figuring this out.

— Reply to this email directly, view it on GitHub https://github.com/tezos-commons/baseDAO/pull/341#discussion_r996524528, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABXQIZXLQNTX3YBQPNTEBFTWDSFWXANCNFSM6AAAAAARAILZ7A . You are receiving this because your review was requested.Message ID: @.***>

sras commented 2 years ago

I really don't understand why the change to test config in haskell/test/Test/Ligo/BaseDAO/Proposal/Flush.hs

That was to make the real network tests to pass in CI. So in this contract, there is some amount of time/level based logic, so sometimes when real network tests fail in CI, we have to tweak the configuration, like here we have increased the duration of the period, and proposal expiry time, so that certain procedures in the tests fall in the expected time/level frame. This is done so that couple of freeze calls at the start of the test will fall in the initial period, and extended the proposal expiry time so that the entire tests runs before the proposal is expired.

I have often thought about a better solution for this, like may be we can provide an interface to the localchain to pause (i mean kill+restart) the baker daemon during the tests so that some control can be had over the block creation so such tests with level sensitive operations can be implemented.

lierdakil commented 2 years ago

That was to make the real network tests to pass in CI

Okay, makes sense. However, I think that should've been a separate commit, with the description more or less explaining what this comment explained (maybe in less detail). Otherwise, the change looks very much out of place amongst purely mechanical API updates.

an interface to the localchain to pause

While possible with local-chain, impossible with testnet, and overall doesn't sound like a great solution (as local-chain is shared amongst all CI jobs).

One thing that might help a bit with timing is running nettests with tasty -j1 option: nettests can't run in parallel anyway, and trying to run those in parallel can lead to arbitrary pauses in random places. On the testnet still might end up breaking though, but time between blocks is significantly larger on the testnet compared to local-chain, so at least level-sensitive tests should work there if they work for local-chain.

sras commented 2 years ago

impossible with testnet

I might be missing something but AFAIU we run on testnets considerably less frequently than we do on localchain. Localchains being shared btw CI jobs is something that we can control, and might even be possible to queue access to it, because the tests can run faster with this hack since we don't have to target delays/periods for the worstcase network performance (hence can use smaller wait times, so for example in BaseDAO, in the place of using 15 levels for one period, we might be able to use 4).