Create starlay_finance_milestone_1-2.md

[Chidasan]

No description provided.

[HCastano]

@Chidasan Hey, have you incorporated any of the feedback, specially around testing, into Milestone 2?

[deepsea514]

Hi, @HCastano , we have solved e2e test problem and will remove ZERO_ADDRESS soon.

[Chidasan]

Hi, @HCastano

We have implemented the feedback received. The e2e test issue has been resolved as @wolfwarrier14 mentioned in the previous comment, and we have also removed the usage of ZERO_ADDRESS in our codebase.
Moreover, we've updated the permalinks.

Let us know if there are any more queries or if there's anything else you would like us to address

[HCastano]

@Chidasan cool, I'll do another review this week then.

Sorry couldn't do it this week, will do Monday/Tuesday

[HCastano]

Hey, so I reviewed your second submission. I'm still concered with the testing situation.

Right now the tests don't pass locally for me:

yarn test:single tests/Pool.spec.ts
...
Test Suites: 1 failed, 1 total
Tests:       44 failed, 44 total
Snapshots:   0 total
Time:        5.827 s, estimated 9 s
Ran all test suites matching /tests\/Pool.spec.ts/i.
Jest did not exit one second after the test run has completed.

'This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with `--detectOpenHandles` to troubleshoot this issue.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

and

 yarn test:single tests/Manager.spec.ts
 ...
 Test Suites: 1 failed, 1 total
Tests:       5 failed, 5 total
Snapshots:   0 total
Time:        2.003 s, estimated 3 s
Ran all test suites matching /tests\/Manager.spec.ts/i.
Jest did not exit one second after the test run has completed.

'This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with `--detectOpenHandles` to troubleshoot this issue.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I also don't see any workflow on GHA which makes use of the integration tests. I'd
consider this to be critical to this submission. Your testing setup is not that
complicated, these integration tests should be run (and should pass) on every PR.

---

Here's a few other points from my review:

• It doesn't look like any of the documentation on the docs.starlay.finance website has
  been updated recently
  • Almost all the pages I checked say "Updated a year ago"
  • I'd expect some documentation around how the liquidation process works since that's
    functionality that was added in this milestone
• In the README it still says you're using [email protected]
  • This should be updated in the README
• There are still missing RustDocs on publc members, please try and make sure that all
  that everything is documented
• There's code in the codebase that's commented out, I'd keep the codebase clean of that
• It would be nice to have an ARCHITECTURE.md file
  • This would be quite helpful to the auditors
  • Not strictly required fro this milestone

[deepsea514]

Hi, @HCastano
Could you give more detail for which tests are not passed?

[HCastano]

@wolfwarrier14 I ran the two suites of tests from the submission document (tests/Pool.spec.ts, tests/Manager.spec.ts).

Before I try and run these locally again can you please send me link to a CI job in which the test suites pass?

[HCastano]

@wolfwarrier14 any updates here?

[Chidasan]

Hi @HCastano,

Apologize for my late reply.
We've updated test and gitbook part. Please check it out: 7a85539

[HCastano]

Thanks for adding the testing suite to the CI! It's gonna make everyone's life easier going forward 😉

Two small nits:

• There are some warnings in your E2E tests, I'd fix those instead of living with them
• Still using a very old Rust nightly, I'd update soon (although I realize this may be a problem stemming from OpenBrush usage)

Milestone looks good to me though 💪

[deepsea514]

Thank you for your approval, @HCastano.
I really appreciate for your notes.
We have tried to remove warnings in JS E2E tests but they are coming from the version mismatch of polkadot-compiler and polkadot-types module.
We have contacted Brushfam for this issue.
And if we upgrade the nightly version, the project causing error because of Openbrush as you expected.

Thank your for your understanding.

[TtomaS7]

@DoubleOTheven please review!

[DoubleOTheven]

Looks good, guys. Nice progress

[TtomaS7]

The payment sent!

https://polkadot.subsquare.io/treasury/child-bounties/19_461
You can claim it in 7 day, thank you!