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

feat: add watched L1 subnet contract to /v2/info #237

Merged
merged 11 commits into from
Mar 15, 2023

Conversation

obycode
Copy link
Member

@obycode obycode commented Mar 11, 2023

Adds this field to the response:

"watch_contract": "ST1PQHQKV0RJXZFY1DGX8MNSNYVE3VGZJSRTPGZGM.subnet"

@codecov-commenter
Copy link

codecov-commenter commented Mar 11, 2023

Codecov Report

Merging #237 (eb43849) into master (54c653c) will increase coverage by 1.06%.
The diff coverage is n/a.

❗ Current head eb43849 differs from pull request most recent head 77c6625. Consider uploading reports for the commit 77c6625 to get more accurate results

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
+ Coverage   91.19%   92.26%   +1.06%     
==========================================
  Files           6        6              
  Lines         284      336      +52     
==========================================
+ Hits          259      310      +51     
- Misses         25       26       +1     
Impacted Files Coverage Δ
core-contracts/contracts/helper/simple-ft.clar 75.00% <ø> (-1.48%) ⬇️
...contracts/contracts/helper/simple-nft-no-mint.clar 66.66% <ø> (ø)
core-contracts/contracts/helper/simple-nft.clar 73.33% <ø> (ø)
core-contracts/contracts/multi-miner.clar 97.05% <ø> (ø)
core-contracts/contracts/subnet.clar 94.96% <ø> (+0.81%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@obycode obycode force-pushed the feat/expose-l1-contract branch from b09285e to 95f1213 Compare March 12, 2023 14:11
@obycode
Copy link
Member Author

obycode commented Mar 12, 2023

Also add RegisterAsset op. Track the asset registration from the L1 (register-new-(n)ft-contract) and trigger a corresponding call in the L2 subnet boot contract. This call also triggers an L2 event for observers. Additionally, the registered contracts are recorded in the contract, allowing calls to (n)ft-withdraw? to error out if the asset is not registered.

@obycode obycode requested review from zone117x and kantai March 13, 2023 13:00
@obycode
Copy link
Member Author

obycode commented Mar 13, 2023

I'll look into these failures today.

Copy link
Contributor

@kantai kantai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me, but the register changes need some end-to-end testing -- I think the "happy path" is tested with the existing deposit + withdraw l1_observe_tests, but we should also test the "unhappy path" where a withdrawal is attempted from a non-registered contract.

@obycode
Copy link
Member Author

obycode commented Mar 13, 2023

we should also test the "unhappy path" where a withdrawal is attempted from a non-registered contract.

Thoughts on where that test should go? I could put it in l1_observer_test.rs, because it will be similar to those tests, but it won't be testing the l1 observer. Is there an existing good place for it?

@kantai
Copy link
Contributor

kantai commented Mar 13, 2023

we should also test the "unhappy path" where a withdrawal is attempted from a non-registered contract.

Thoughts on where that test should go? I could put it in l1_observer_test.rs, because it will be similar to those tests, but it won't be testing the l1 observer. Is there an existing good place for it?

l1_observer_test is probably the closest to good existing place. You could add a sibling module to tests::l1_observer_test like tests::l1_withdrawal which just imports the necessary utility methods, etc. from l1_observer_test.

@obycode obycode force-pushed the feat/expose-l1-contract branch from 4e6c297 to 9d1d8cc Compare March 13, 2023 20:49
@obycode obycode requested a review from kantai March 14, 2023 13:49
obycode added 6 commits March 14, 2023 09:49
Adds this field to the response:
```
"watch_contract": "ST1PQHQKV0RJXZFY1DGX8MNSNYVE3VGZJSRTPGZGM.subnet"
```
Track the asset registration from the L1 (`register-new-(n)ft-contract`)
and trigger a corresponding call in the L2 subnet boot contract. This
call also triggers an L2 event for observers. Additionally, the
registered contracts are recorded in the contract, allowing calls to
`(n)ft-withdraw?` to error out if the asset is not registered.
If a user tries to withdraw an unregistered asset from the L2, the
withdrawal should fail and they should retain ownership.
@obycode obycode force-pushed the feat/expose-l1-contract branch from d60260d to 0113342 Compare March 14, 2023 13:49
@obycode
Copy link
Member Author

obycode commented Mar 14, 2023

The last commit looks unrelated, but it is just cleaning up a bunch of warnings that were annoying during the build/test process.

@obycode obycode force-pushed the feat/expose-l1-contract branch from e84d4c3 to cd89d2c Compare March 14, 2023 18:32
@obycode obycode force-pushed the feat/expose-l1-contract branch from cd89d2c to 48a1617 Compare March 14, 2023 23:03
@obycode obycode force-pushed the feat/expose-l1-contract branch from bbda511 to eb43849 Compare March 15, 2023 00:59
@obycode
Copy link
Member Author

obycode commented Mar 15, 2023

Ok @kantai, this is ready for a final review.

@zone117x please also take a look at the updated events and let me know if that will be okay for the API or if you request any changes.

Copy link
Member

@zone117x zone117x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @obycode this is working as expected

Copy link
Contributor

@kantai kantai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

You should make sure the new test module gets picked up by the test matrix step in github CI, though.

In .github/workflows/ci.yml, on step compute-layer-1-tests, it currently does:

          cargo test --workspace --bin=subnet-node -- l1_ --list --format=terse | sed -e 's/: test//g' | jq -ncR '{"test-name": [inputs]}' > test_names.json

to create a JSON struct listing the tests, but the cargo test filter l1_ is too restrictive for this. I think just changing that to l would work:

          cargo test --workspace --bin=subnet-node -- l --list --format=terse | sed -e 's/: test//g' | jq -ncR '{"test-name": [inputs]}' > test_names.json

obycode added 2 commits March 15, 2023 12:43
Also refactored a bit to reduce code duplication.
This ensures that the l2_withdrawal test gets picked up as well.
@obycode obycode force-pushed the feat/expose-l1-contract branch from 1f64a80 to 77c6625 Compare March 15, 2023 16:43
@obycode
Copy link
Member Author

obycode commented Mar 15, 2023

Thanks @kantai - I made that CI change in the latest commit.

@zone117x I added the txid to the event as requested. Can you take one more look at that?

@obycode obycode merged commit 0adbbe1 into master Mar 15, 2023
@obycode obycode deleted the feat/expose-l1-contract branch March 15, 2023 19:01
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

Successfully merging this pull request may close these issues.

4 participants