-
Notifications
You must be signed in to change notification settings - Fork 16
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 UpdateAdmin
and ClearAdmin
constructors in the Remote
#438
feat: Add UpdateAdmin
and ClearAdmin
constructors in the Remote
#438
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/replies #438 +/- ##
================================================
- Coverage 72.20% 72.18% -0.02%
================================================
Files 56 56
Lines 3457 3484 +27
================================================
+ Hits 2496 2515 +19
- Misses 961 969 +8 ☔ View full report in Codecov by Sentry. |
735bfeb
to
8b74359
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.
LGTM! I only wonder if doc comments would be useful with these new pub methods
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.
One comment about API improvement.
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.
I like this change, it's nice to have those extra methods in the Remote
implementation. The only thing I spend more time was checking the remote.rs
file with tests. I realized it is more complicated than it can be. I know it's not directly related to this PR, but we can do a nice improvement with low cost:
unsigned_contract
andsigned_contract
are pretty much the same - let's remove the code duplication and make a singleCounter
contract generic over the counter type. We can then addtype SignedContract = Counter<i32>; type UnsignedContract = Counter<u32>;
.- Names
signed
andunsigned
reminds me of signing something with some cryptographic tool, when in this context it meansi32
andu32
. Let's change it toCounterI32
/CounterU32
orContractI32
/ContractU32
instead. ContractStorage
is a dead code - to be removed.- Remove the
#[entry_points(generics<i32>)]
- we don't need it in the test scenarios. Moreover it might be misleading for other devs:
- it's only
i32
, but we can support many other types - better way would be to add feature flags for the contracts to choose between entry points types, - it gives a hint that it always should be present with sylvia contracts - it doesn't have to.
.unwrap(); | ||
|
||
// Set manager contract as an admin of the counter contract | ||
app.app_mut() |
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.
Let's create a new setup function for the admin scenarios
I agree it could be simplified
I can see that. Let's change it later.
This is intentional. There was a bug where it was impossible to store
It provides the coverage though. |
3aa9968
to
789266f
Compare
789266f
to
6fb6049
Compare
No description provided.