-
Notifications
You must be signed in to change notification settings - Fork 12
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
Conversation
Codecov Report
@@ 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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
b09285e
to
95f1213
Compare
Also add |
I'll look into these failures today. |
There was a problem hiding this 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.
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? |
|
4e6c297
to
9d1d8cc
Compare
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.
d60260d
to
0113342
Compare
The last commit looks unrelated, but it is just cleaning up a bunch of warnings that were annoying during the build/test process. |
e84d4c3
to
cd89d2c
Compare
cd89d2c
to
48a1617
Compare
bbda511
to
eb43849
Compare
There was a problem hiding this 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
There was a problem hiding this 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
Also refactored a bit to reduce code duplication.
This ensures that the l2_withdrawal test gets picked up as well.
1f64a80
to
77c6625
Compare
Adds this field to the response: