Fix blank transition builder #108
Conversation
crisdut
force-pushed
the
feat/improviments
branch
from
c436445
to
a94dd7a
Compare
Codecov Report
@@ Coverage Diff @@
## master #108 +/- ##
========================================
- Coverage 26.5% 26.2% -0.3%
========================================
Files 39 39
Lines 4473 4523 +50
========================================
- Hits 1187 1186 -1
- Misses 3286 3337 +51
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -477,8 +477,9 @@ impl DivisibleAssetSpec { | |||
| pub fn details(&self) -> Option<&str> { self.naming.details.as_ref().map(|d| d.as_str()) } | |||
| } | |||
|
|
|||
| #[derive(Clone, Eq, PartialEq, Debug, Default)] | |||
| #[derive(StrictType, StrictEncode, StrictDecode)] | |||
| #[derive(Wrapper, Clone, Ord, PartialOrd, Eq, PartialEq, Hash, From, Debug)] | |||
There was a problem hiding this comment.
Why Default implementation is removed?
There was a problem hiding this comment.
Default causes conflict with strict_dumb. Do you prefer maintain Default?
There was a problem hiding this comment.
I think having default is better
| #[derive(Clone, Eq, PartialEq, Debug, Default)] | ||
| #[derive(StrictType, StrictEncode, StrictDecode)] | ||
| #[derive(Wrapper, Clone, Ord, PartialOrd, Eq, PartialEq, Hash, From, Debug)] | ||
| #[wrapper(Deref, Display)] |
There was a problem hiding this comment.
Since we are adding Wrapper we need to add FromStr here and remove manual FromStr implementation below
| impl RicardianContract { | ||
| pub fn from_strict_val_unchecked(value: &StrictVal) -> Self { | ||
| let s = value.unwrap_string(); | ||
| RicardianContract::from_str(&s).expect("invalid term data") |
There was a problem hiding this comment.
This will open a vulnerability where an attacker can send a malformed consignment and crash end-user wallets and nodes (DoS attack).
There was a problem hiding this comment.
I didn't know, as I saw in several code snippets, I imagined that this was the correct implementation. What would be the alternative?
There was a problem hiding this comment.
Why do you need an unchecked version?
There was a problem hiding this comment.
I have a method that extracts some information from the ContractIface::global method, and this method returns the StrictVal array. I use from_strict_val_unchecked to convert StrictVal to a specific structure.
There was a problem hiding this comment.
Can you point specific place in code where you do that call?
There was a problem hiding this comment.
There was a problem hiding this comment.
Why you do not want to use try_from and just propagate the error?
| pub fn data( | ||
| &self, | ||
| name: impl Into<FieldName>, | ||
| ) -> Result<LargeVec<DataAllocation>, ContractError> { |
There was a problem hiding this comment.
I think we need to add outpoint filter like in fungible method above
| @@ -101,6 +101,9 @@ impl Allocation { | |||
| pub fn with(index: TokenIndex, fraction: OwnedFraction) -> Allocation { | |||
| Allocation(index, fraction) | |||
| } | |||
|
|
|||
| pub fn token_id(self) -> TokenIndex { self.0 } | |||
There was a problem hiding this comment.
Id's are usually hashes, thus better to use the type name token_index
|
Can you please rebase on the master to reflect everything which has happened in v0.11 branch? |
Description
This PR intent fix a blank transition builder when contract has another
ifaceimpl. This particular case occurs when issuer issues many contracts with differentifaceimplin the same UTXO.CC @cryptoquick @dr-orlovsky