Open shawnxie999 opened 1 month ago
codecov-commenter
commented
1 month ago Attention: Patch coverage is 50.00000% with 3 lines in your changes are missing coverage. Please review.
Project coverage is 70.9%. Comparing base (
5aa1106) to head (2c0572c). Report is 2 commits behind head on develop.
gregtatcam
commented
1 month ago Does it make sense to add another virtual method to STBase, say canInsertMetadata() with the default implementation returning isDefault() and STIssue/STCurrency returning false? canInsertMetadata() is only called in the metadata handling. This ensures that isDefault() semantic is not broken.
shawnxie999
commented
1 month ago Does it make sense to add another virtual method to
STBase, saycanInsertMetadata()with the default implementation returningisDefault()andSTIssue/STCurrencyreturning false?canInsertMetadata()is only called in the metadata handling. This ensures thatisDefault()semantic is not broken.
I'm not sure if that would be needed. isDefault() itself is only used for metadata purposes(though would need to double check this), so I don't think we would need to introduce another method for this. And by Ed's definition, isDefault() is only supposed to return true if the value represents "nothing", so omitting XRP would clearly be wrong behavior.
gregtatcam
commented
1 month ago Does it make sense to add another virtual method to
STBase, saycanInsertMetadata()with the default implementation returningisDefault()andSTIssue/STCurrencyreturning false?canInsertMetadata()is only called in the metadata handling. This ensures thatisDefault()semantic is not broken.I'm not sure if that would be needed.
isDefault()itself is only used for metadata purposes(though would need to double check this), so I don't think we would need to introduce another method for this. And by Ed's definition,isDefault()is only supposed to return true if the value represents "nothing", so omitting XRP would clearly be wrong behavior.
Can you confirm it's only used for the metadata?
Does badCurrency() represent "nothing"? By the way, STAmount has isDefault() as return (mValue == 0) && mIsNative;; i.e. XRP(0), which is clearly not "nothing".
exunda
commented
1 month ago Does it make sense to add another virtual method to
STBase, saycanInsertMetadata()with the default implementation returningisDefault()andSTIssue/STCurrencyreturning false?canInsertMetadata()is only called in the metadata handling. This ensures thatisDefault()semantic is not broken.I'm not sure if that would be needed.
isDefault()itself is only used for metadata purposes(though would need to double check this), so I don't think we would need to introduce another method for this. And by Ed's definition,isDefault()is only supposed to return true if the value represents "nothing", so omitting XRP would clearly be wrong behavior.Can you confirm it's only used for the metadata?
Does
badCurrency()represent "nothing"? By the way,STAmounthasisDefault()asreturn (mValue == 0) && mIsNative;; i.e. XRP(0), which is clearly not "nothing".
That is really a scary question for a Rippel employee to ask! 😨 The badCurrency is the encoding of the string XRP as 160-bit asset code and used to prevent trust lines with that asset code from being created, probably as a safety to prevent people from being tricked. But attackers can still do this trick by using other character case or even a space. Very sloppy. I am surprsied nobody else has discussed this!
Bronek
commented
4 weeks ago FWIW, I am uncomfortable with this change. Both STCurrency and STIssue are basic protocol building blocks, and because of this we cannot assume that isDefault() functions will be only ever used for metadata generation (that is, regardless of the answer to the question posed by @gregtatcam , things may change in the future). IMO we should either add a new function with the required semantics and change other bits (i.e. where metadata is generated) to use it, or do something else.
dangell7
commented
4 weeks ago FWIW, I am uncomfortable with this change. Both
STCurrencyandSTIssueare basic protocol building blocks, and because of this we cannot assume thatisDefault()functions will be only ever used for metadata generation (that is, regardless of the answer to the question posed by @gregtatcam , things may change in the future). IMO we should either add a new function with the required semantics and change other bits (i.e. where metadata is generated) to use it, or do something else.
I agree with this.
Here is what we did:
bool const recordDefaultAmounts = to.rules().enabled(fixXahauV1);
...
bool const shouldRecord =
(obj.getSType() == STI_AMOUNT && recordDefaultAmounts) ||
!obj.isDefault();FWIW, I am uncomfortable with this change. Both
STCurrencyandSTIssueare basic protocol building blocks, and because of this we cannot assume thatisDefault()functions will be only ever used for metadata generation (that is, regardless of the answer to the question posed by @gregtatcam , things may change in the future). IMO we should either add a new function with the required semantics and change other bits (i.e. where metadata is generated) to use it, or do something else.I agree with this.
Here is what we did:
bool const recordDefaultAmounts = to.rules().enabled(fixXahauV1); ... bool const shouldRecord = (obj.getSType() == STI_AMOUNT && recordDefaultAmounts) || !obj.isDefault();
Having read https://github.com/XRPLF/rippled/issues/4965#issuecomment-2079688630 I think I was mistaken. The semantics of isDefault() is defined in STBase ("does the object hold any useful data ?"); neither STIssue nor STCurrency implement it correctly, as of today. Having said that, I also think that the proposed amendment is badly named, as this is not an AMM amendment. It is a currency/IOU metadata amendment.
dangell7
commented
3 weeks ago FWIW, I am uncomfortable with this change. Both
STCurrencyandSTIssueare basic protocol building blocks, and because of this we cannot assume thatisDefault()functions will be only ever used for metadata generation (that is, regardless of the answer to the question posed by @gregtatcam , things may change in the future). IMO we should either add a new function with the required semantics and change other bits (i.e. where metadata is generated) to use it, or do something else.I agree with this. Here is what we did:
bool const recordDefaultAmounts = to.rules().enabled(fixXahauV1); ... bool const shouldRecord = (obj.getSType() == STI_AMOUNT && recordDefaultAmounts) || !obj.isDefault();Having read #4965 (comment) I think I was mistaken. The semantics of
isDefault()is defined inSTBase("does the object hold any useful data ?"); neitherSTIssuenorSTCurrencyimplement it correctly, as of today. Having said that, I also think that the proposed amendment is badly named, as this is not an AMM amendment. It is a currency/IOU metadata amendment.
I disagree as STAmount clearly defines the isDefault() as return (mValue == 0) && mIsNative; so if that is incorrect then yeah it should just be false AND STAmount should be fixed as well, however if that is the correct default response then STIssue and STCurrency are correct in their response and this fix needs to be handled outside of the isDefault() behavior.
But alas I really don't have any firm position on how its handled in ripple(d).
Bronek
commented
3 weeks ago
return (mValue == 0) && mIsNative;
I think the condition that you are citing here just happens to match the definition of "no useful data here", in the context of STAmount.
High Level Overview of Change
In the
NewFieldsof a AMM object after aAMMCreatetransaction,Assetwould be omitted if it is XRP, which is incorrect. A new amendment fixAMMMetadata fixes this problemContext of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)