Conversation
* refactor(lk): stream funder * nitpick comment --------- Co-authored-by: Andrei Vlad Birgaoanu <andreivladbrg@gmail.com>
* feat: more MI campaign data on-chain * remove token decimas from state * test: fix defaultCampaignData() --------- Co-authored-by: Andrei Vlad Birgaoanu <andreivladbrg@gmail.com>
andreivladbrg
left a comment
There was a problem hiding this comment.
to be more efficient, i'll leave comments one by one if something catches my attention
would it be possible to place trident-tests under the current tests dir so that we have:
tests/
├── anchor
└── trident
refactor(tests): remove the fuzz_0 dir
Thanks for the suggestion! I've spent some time trying to make the above hierarchy work, tried a bunch of things, but failed to do so, because there seem to be some internal path dependencies rejecting a different folder structure - both for the Anchor Feel free to give this a try yourself, if you want, though. |
|
i am not familiar with trident, but in case of anchor, they should definitely work by updating the vitest config file with the new path |
In the Anchor tests case, there was some issue with the Chainlink test fixture that couldn't be properly configured (I did update the |
andreivladbrg
left a comment
There was a problem hiding this comment.
since you are still making changes, i am going to leave these comments, and continue adding more
we should add *.fuzz-artifacts in .gitignore file
| @@ -0,0 +1,19 @@ | |||
| [workspace] | |||
There was a problem hiding this comment.
do we need this if we have no field configured?
There was a problem hiding this comment.
That's, likely, a leftover from the automatic Trident file generation.
We can remove it.
| address = "99B2bTijsU6f1GCT73HmdR7HCFFjGMBcPZY6jZ96ynrR" | ||
| filename = "accounts/chainlink_sol_usd_feed_mock.json" | ||
|
|
||
| # [fuzz.coverage] |
There was a problem hiding this comment.
since they are commented, can we remove them?
There was a problem hiding this comment.
Yes, we can add them again when/if we need the coverage.
| self.trident.get_client().warp_to_timestamp(GENESIS.try_into().unwrap()); | ||
| } | ||
|
|
||
| #[flow] |
There was a problem hiding this comment.
why not create a dir called flows and implement there the logic for each Ix flow ?
There was a problem hiding this comment.
it would be a good idea to have a just command to run the trident tests and also autogen this file (and maybe others as well)
There was a problem hiding this comment.
Not sure about the auto generation, but the just script has been added in 835a58e
There was a problem hiding this comment.
Not sure about the auto generation
it would be similar to the errors & types from the anchor tests
we would need something like this before running trident fuzz run ...:
cp ../programs/lockup/src/utils/lockup_math.rs fuzz_lockup/helpers/lockup_math.rsThere was a problem hiding this comment.
I'm open to the idea
| #[flow] | ||
| fn flow_withdraw(&mut self) { | ||
| // First, create the stream we're going to cancel | ||
| let (cwt_ix_accs, _) = self.create_with_timestamps(); |
There was a problem hiding this comment.
can't we create streams in the fn start?
There was a problem hiding this comment.
That's how it was done in the beginning, until I realized that creating the streams in start() makes it impossible to run flow_create_with_timestamps() - and, possibly, flow_create_with_durations(), as well.
There was a problem hiding this comment.
but, how can it initialize the program then?
There was a problem hiding this comment.
Program initialization has nothing to do with this, as it only happens once.
The problem here is that the create_with_timestamps()/create_with_durations() fn would be called twice:
- both inside
start() - and inside its respective flow.
Executing the same Ix twice during a single Fuzz iteration leads to a failure, because fuzzing it a second times yields the same values as the first one (as the fuzzing seed is per iteration, not per ix call).
There was a problem hiding this comment.
Executing the same Ix twice during a single Fuzz iteration leads to a failure, because fuzzing it a second times yields the same values as the first one (as the fuzzing seed is per iteration, not per ix call).
let's talk about this in person
There was a problem hiding this comment.
lets change the test command to run both anchor & trident tests
and then have individual commands for: test-anchor and test-trident
| cancel_ix_accs.stream_data.set_address(cwt_ix_accs.stream_data.pubkey()); | ||
| cancel_ix_accs.stream_data_ata.set_address(cwt_ix_accs.stream_data_ata.pubkey()); | ||
| cancel_ix_accs.stream_nft_mint.set_address(cwt_ix_accs.stream_nft_mint.pubkey()); | ||
| cancel_ix_accs.deposited_token_program.set_address(cwt_ix_accs.deposit_token_program.pubkey()); |
There was a problem hiding this comment.
can't we set all these accounts in the instructions/cancel.rs#InstructionHooks.set_accounts (similar to create tests)?
the objective is simple - make the test_fuzz.rs the entrypoint to the fuzz tests similar to our lib.rs with minimal logic
we would have declared only the:
- setup
- declaration of flows
- exec the tx
There was a problem hiding this comment.
The Cancel Ix isn't aware - in its Ix Hooks - of the specific accounts that the Fuzzer has configured the Create Ix with.
At the same time, if we don't configure them manually (using the accs from the Create Ix) and just let the Cancel Ix fuzz them itself, there will, inevitably, be many mismatches resulting in failed Cancel txs (because, e.g. the stream_data accounts in Create and Cancel Ixs are different)
There was a problem hiding this comment.
The
CancelIx isn't aware - in its Ix Hooks - of the specific accounts that the Fuzzer has configured the Create Ix with.
hmm, you sure? can't the accounts can be obtained from FuzzAccounts struct?
There was a problem hiding this comment.
can't the accounts can be obtained from FuzzAccounts struct
Tapping into the FuzzAccounts struct inside the set_accounts() Hook is, theoretically, possible, but this'd require:
- calling the
get_or_create...()fns from the underliningAccountsStorage - and passing the account id, Trident, PDA seeds & the account metadata to it (even though the account has already been created by the Create Ix),
- basically, duplicating the account config code from the fuzzed Create Ix.
This doesn't sound reasonable.
There was a problem hiding this comment.
Tapping into the
FuzzAccountsstruct inside theset_accounts()Hook is, theoretically, possible, but this'd require:
wdym?
what is this for then?
There was a problem hiding this comment.
That's just a type wrapper - and it doesn't affect/change what I said above.
| # Run Lockup fuzz tests | ||
| [group("fuzz")] | ||
| fuzz-lockup: | ||
| cd trident-tests && trident fuzz run fuzz_lockup |
There was a problem hiding this comment.
its not a good approach to use cd
it would be better to declare a justfile inside trident-tests for this command: trident fuzz run fuzz_lockup
and then edit this (root justfile) to:
fuzz-lockup:
just --justfile trident-tests/justfile fuzz_lockupfor example, take a look how we handle the justfiles in the monorepo
There was a problem hiding this comment.
Didn't know that just files can be chained like that.
Sounds good.
| } | ||
|
|
||
| /// Warps to a random timestamp between now and 2x the total stream duration | ||
| fn warp_to_a_random_future_time(trident: &mut Trident, stream_data_pubkey: &Pubkey) -> u64 { |
There was a problem hiding this comment.
can't we declare this inside the impl FuzzTest? so that we remove the param trident: &mut Trident
| ); | ||
|
|
||
| // If the withdrawable amount is zero, skip the withdrawal | ||
| if withdrawable_amount == 0 { |
There was a problem hiding this comment.
we should never skip the withdrawal since this test logic must ensure on each run a withdrawal happens
with this pattern there are basically discared test runs
we should always warp between: (start + 1, > start + 1)
There was a problem hiding this comment.
this point applies to cancel as well
There was a problem hiding this comment.
we should always warp between: (start + 1, > start + 1)
This wouldn't be enough to make sure that the withdrawable amount is > 0 at the warped time, because:
- there might be a cliff (or not),
- the start unlock amount may be 0 (or not),
- etc.
Taking this all into consideration and making the time warping dependant on all this just to have 0 discarded test runs is not worth it, imo.
|
@IaroslavMazur i implemented the 0.12.0 update here: #312 i also added an explanation there for the motivation. would you be ok with merging that PR and then closing this one in its favor, or would you prefer a different approach? |
Closes #298