Change TxBuilder so it has access to the whole TxBodyContent instead … #140
Conversation
…of just the inputs. Add some utility functions to index into mints and withdrawls.
|
I want to add some tests for the stake validator (not 100% sure how easy to add it would be), but wanted to hear any feedback in the meantime. |
| _Value :: Iso' Value (Map AssetId Quantity) | ||
| _Value = iso from to where | ||
| -- the 'Value' constructor is not exposed so we have to take the long way around | ||
| from = Map.fromList . C.valueToList | ||
| to = C.valueFromList . Map.toList | ||
|
|
||
| _AssetId :: Prism' C.AssetId (C.PolicyId, C.AssetName) |
There was a problem hiding this comment.
Didn't end up using this, but thought it might be ok to keep in.
| { txiSpendingInputs = Map.fromList (fmap (view L._BuildTxWith) <$> txIns) | ||
| , txiReferenceInputs = Set.fromList (view L._TxInsReference txInsReference) | ||
| } | ||
| type TxBody = C.TxBodyContent C.BuildTx C.BabbageEra |
There was a problem hiding this comment.
type aliases don't expand consistently. Please either use the long form or a newtype
There was a problem hiding this comment.
Happy to change it if you feel strongly about this, but I generally think that it might occasionally be a good tradeoff. In this case there would be a bunch of C.TxBodyContent C.BuildTx C.BabbageEra all over the place making the types less readable imo.
There was a problem hiding this comment.
I'm for keeping this too, IMO the threshold for newtypes should be if we have several different synonyms for the same type, or if we want to distinguish values that have been validated in some way.
type aliases don't expand consistently
Is this a tooling issue (hls) or GHC itself?
There was a problem hiding this comment.
for partial functions (findIndexWithdrawl), can we prefix them with try? Like tryFindINdexWithdrawl? Just good practice to indicate where things are partial
There was a problem hiding this comment.
Ooft. Data.Map.findIndex just errors? Why so much partiality 😭
There was a problem hiding this comment.
I'll leave this up to @j-mueller to decide. I usually like to call things tryXYZ if they error, XYZ if they return a Maybe or Either. I think this made it into our styleguide, but obviously once we start bringing in other libraries we might prefer to keep in line with their conventions.
There was a problem hiding this comment.
Yeah, I generally agree. I added a note in the comments that all these functions are partial, but decided to keep consistent naming with the existing ones for now.
| Note: cardano-api represents a value as a @Map AssetId Quantity@, this is different than the on-chain representation | ||
| which is @Map CurrencySymbol (Map TokenName Quantity). | ||
| Here, we want to get the index into the on-chain map, but instead index into the cardano-api @Map CurrencySymbol Witness@. | ||
| These two indexes should be the same by construction, but it is possible to violate this invariant when building a tx. |
There was a problem hiding this comment.
I think it might be possible to violate the invariant when building the Tx, but I don't think we could ever have a situation where such a Tx was able to actually execute on chain. I'm not positive, though; maybe we should try it in an integration test.
There was a problem hiding this comment.
Given the discussion we had, maybe this comment is a bit overkill. The Value from the txmint will always be sorted, and all the cs also appear in this map, so it should always be fine to use this. As @peter-mlabs said only invalid txs can invalidate this
There was a problem hiding this comment.
Agree that only invalid transactions can violate the invariant, but what I'm wondering is: Can this construction prevent us from building a valid transaction in a case where it should be possible to build one?
Setting up staking validator tests is a bit more involved than expected, but there is some work going on for this already. So we thought it might be ok to postpone writing these tests until we have that machinery in place. |
| Note: cardano-api represents a value as a @Map AssetId Quantity@, this is different than the on-chain representation | ||
| which is @Map CurrencySymbol (Map TokenName Quantity). | ||
| Here, we want to get the index into the on-chain map, but instead index into the cardano-api @Map CurrencySymbol Witness@. | ||
| These two indexes should be the same by construction, but it is possible to violate this invariant when building a tx. |
There was a problem hiding this comment.
Agree that only invalid transactions can violate the invariant, but what I'm wondering is: Can this construction prevent us from building a valid transaction in a case where it should be possible to build one?
| { txiSpendingInputs = Map.fromList (fmap (view L._BuildTxWith) <$> txIns) | ||
| , txiReferenceInputs = Set.fromList (view L._TxInsReference txInsReference) | ||
| } | ||
| type TxBody = C.TxBodyContent C.BuildTx C.BabbageEra |
There was a problem hiding this comment.
I'm for keeping this too, IMO the threshold for newtypes should be if we have several different synonyms for the same type, or if we want to distinguish values that have been validated in some way.
type aliases don't expand consistently
Is this a tooling issue (hls) or GHC itself?
|
@nazrhom please also port this to the |
…of just the inputs.
Add some utility functions to index into mints and withdrawls.