Refactor P-chain Builder #3282
Refactor P-chain Builder #3282
Conversation
|
|
||
| // Join merges the provided slices into a single slice. | ||
| // | ||
| // TODO: Use slices.Concat once the minimum go version is 1.22. |
There was a problem hiding this comment.
(no action required) Their signature is interesting:
func Concat[S ~[]E, E any](slices ...S) S {Without looking too deeply I guess it's to play nicely with other functions in that package / make it easier to use them together without being confused about which type is which.
wallet/chain/p/builder/builder.go
Outdated
| @@ -677,6 +713,33 @@ func (b *builder) NewImportTx( | |||
| }) | |||
| } | |||
|
|
|||
| var ( | |||
| toBurn map[ids.ID]uint64 | |||
There was a problem hiding this comment.
This section can be simplified further.
toBurn := map[ids.ID]uint64{}
toStake := map[ids.ID]uint64{}
var excessAVAX uint64
if avax := importedAmounts[avaxAssetID]; avax > txFee {
excessAVAX = avax - txFee
} else {
toBurn[avaxAssetID] = txFee - avax
}There was a problem hiding this comment.
Sorry, your explanation made me realise that it should be avax >= txFee.
There was a problem hiding this comment.
I think the code actually works regardless of avax >= txFee or avax > txFee (as setting toBurn[avaxAssetID] = 0 is fine... Although slightly inefficient I suppose.
There was a problem hiding this comment.
Agreed, it should be functionally identical. Just feels "more correct" to leave excessAVAX == 0 instead of also adding a zero burn.
| ) | ||
|
|
||
| utxosWithAssetID, utxosWithOtherAssetID := splitUTXOsByLocktime(utxos, unlockedTime) | ||
| require.Equal(expectedUnlockedUTXOs, utxosWithAssetID) |
There was a problem hiding this comment.
This is an example of where I strongly believe that assert should be used over require. The failure of this comparison doesn't necessarily corrupt the next one, and it may very well be useful to know if/how they fail simultaneously.
Same applies to the next test.
There was a problem hiding this comment.
I know that you prefer the usage of assert for things like this... But currently the status-quo in avalanchego is to ban usage of assert. (And this PR isn't going to change the linting rules to allow assert)
Why this should be merged
It seems like #3232 is daunting to review - this PR further reduces that diff by refactoring the builder without adding complexity in yet.
How this works
ImportTxto always callspend.spendlast.spendHelperto encapsulate common operations performed inspendHow this was tested