`fixInnerObjTemplate`: Add STObject constructor to explicitly set inner object template #4906

gregtatcam · 2024-01-29T22:10:03Z

High Level Overview of Change

Add an STObject constructor overload that includes an additional bool argument to set the inner object template. Set the argument to true if the constructed object is an inner object and if the fixInnerObjTemplate amendment is enabled.

Context of Change

Currently, the inner object template is not set upon object creation. If the object contains an soeDEFAULT field, and this field is initially set to the default value, accessing the field results in tefEXCEPTION error. Conversely, if the top object's soeDEFAULT field, initially set to the default value, is accessed, it returns the default value since top objects have the template set during construction. This fix aims to make the inner object behavior consistent with the top object behavior. The inner object must be explicitly constructed, as described in the summary, for this fix to work.

Currently, two AMM fields, sfTradingFee and sfDiscountedFee, included in the inner objects sfVoteEntry and sfAuctionSlot respectively, are declared as soeDEFAULT fields. This fix employs the new STObject constructor when constructing inner objects that include these fields.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Performance (increase or change in throughput and/or latency)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

Test Plan

AMM unit-test is extended with a test, which demonstrates the behavior before and after this fix.

codecov-commenter · 2024-01-30T13:25:07Z

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (6d3c21e) 61.50% compared to head (f2677a3) 61.47%.

Files	Patch %	Lines
src/ripple/app/tx/impl/AMMBid.cpp	14.28%	3 Missing and 3 partials ⚠️
src/ripple/app/misc/impl/AMMUtils.cpp	88.88%	0 Missing and 1 partial ⚠️
src/ripple/app/tx/impl/AMMVote.cpp	75.00%	0 Missing and 1 partial ⚠️
src/ripple/protocol/impl/STObject.cpp	92.30%	0 Missing and 1 partial ⚠️
src/ripple/rpc/handlers/AMMInfo.cpp	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4906      +/-   ##
===========================================
- Coverage    61.50%   61.47%   -0.04%     
===========================================
  Files          797      797              
  Lines        70132    70156      +24     
  Branches     36245    36261      +16     
===========================================
- Hits         43137    43127      -10     
- Misses       19756    19786      +30     
- Partials      7239     7243       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/ripple/app/misc/impl/AMMUtils.cpp

src/ripple/protocol/impl/STObject.cpp

src/ripple/protocol/SOTemplate.h

* Add comment to InnerObjectFormats * Use if initializer in makeInnerObject() * Add assert to STObject::set()

ximinez

Looks good. One optional comment to consider.

src/ripple/app/misc/impl/AMMUtils.cpp

danielwwf · 2024-02-03T20:59:37Z

After talking with some people, we would like to see a 2.0.2 for this fix that gets pushed ASAP. Waiting for 2.1.0 seems too long IMHO.

Departing AMM and putting up an AMMv2 seems the best way forward to avoid another XLS20d situation.

* Create AuctionSlot on AMMCreate and update on AMMDeposit * Add assert to check for AuctionSlot

gregtatcam · 2024-02-06T19:14:02Z

Looks good. One optional comment to consider.

There is another update. Please re-review. Thanks.

src/ripple/app/misc/impl/AMMUtils.cpp

src/ripple/app/tx/impl/AMMBid.cpp

src/ripple/app/tx/impl/AMMVote.cpp

src/ripple/protocol/impl/STObject.cpp

src/ripple/rpc/handlers/AMMInfo.cpp

* Remove comments * Refactor amendment changed code

* Change STObject::set() arg to rvalue ref

seelabs

👍

dangell7 · 2024-02-07T15:18:52Z

Why don't we just make these fields soeOPTIONAL? Whats the purpose of these being soeDEFAULT?

Also you stated If the object contains an soeDEFAULT field, and this field is initially set to the default value, accessing the field results in tefEXCEPTION error.

Isn't this exactly what default behavior is supposed to be? soeDEFAULT // optional, if present, must not have default value

Thanks

gregtatcam · 2024-02-07T15:23:45Z

Why don't we just make these fields soeOPTIONAL? Whats the purpose of these being soeDEFAULT?

Also you stated If the object contains an soeDEFAULT field, and this field is initially set to the default value, accessing the field results in tefEXCEPTION error.

Isn't this exactly what default behavior is supposed to be? soeDEFAULT // optional, if present, must not have default value

Thanks

Changing the field's style is risky and it's a lot more code changes.

soeDEFAULT field saves space. If it has the default value then it should not be present. But if it is accessed then it should return the default value if it's not present.

dangell7 · 2024-02-07T15:29:25Z

Why don't we just make these fields soeOPTIONAL? Whats the purpose of these being soeDEFAULT?
Also you stated If the object contains an soeDEFAULT field, and this field is initially set to the default value, accessing the field results in tefEXCEPTION error.
Isn't this exactly what default behavior is supposed to be? soeDEFAULT // optional, if present, must not have default value
Thanks

Changing the field's style is risky and it's a lot more code changes.

soeDEFAULT field saves space. If it has the default value then it should not be present. But if it is accessed then it should return the default value if it's not present.

Ah I see thank you

src/ripple/app/misc/impl/AMMUtils.cpp

src/test/jtx/impl/AMM.cpp

* Refactor auction slot amendment creation on initialize * Update a comment in amm test utils

intelliot · 2024-02-07T20:07:27Z

Suggested commit message:

`fixInnerObjTemplate`: set inner object template (#4906)

Add `STObject` constructor to explicitly set the inner object template.
This allows certain AMM transactions to apply in the same ledger:

There is no issue if the trading fee is greater than or equal to 0.01%.
If the trading fee is less than 0.01%, then:
- After AMM create, AMM transactions must wait for one ledger to close
  (3-5 seconds).
- After one ledger is validated, all AMM transactions succeed, as
  appropriate, except for AMMVote.
- The first AMMVote which votes for a 0 trading fee in a ledger will
  succeed. Subsequent AMMVote transactions which vote for a 0 trading
  fee will wait for the next ledger (3-5 seconds). This behavior repeats
  for each ledger.

This has no effect on the ultimate correctness of AMM. This amendment
will allow the transactions described above to succeed as expected, even
if the trading fee is 0 and the transactions are applied within one
ledger (block).

intelliot · 2024-02-17T01:16:18Z

@legleux reviewing for QA

Add `STObject` constructor to explicitly set the inner object template. This allows certain AMM transactions to apply in the same ledger: There is no issue if the trading fee is greater than or equal to 0.01%. If the trading fee is less than 0.01%, then: - After AMM create, AMM transactions must wait for one ledger to close (3-5 seconds). - After one ledger is validated, all AMM transactions succeed, as appropriate, except for AMMVote. - The first AMMVote which votes for a 0 trading fee in a ledger will succeed. Subsequent AMMVote transactions which vote for a 0 trading fee will wait for the next ledger (3-5 seconds). This behavior repeats for each ledger. This has no effect on the ultimate correctness of AMM. This amendment will allow the transactions described above to succeed as expected, even if the trading fee is 0 and the transactions are applied within one ledger (block).

Add STObject constr option to explicitly set inner obj template

a4ee2b8

gregtatcam requested review from ximinez, seelabs and scottschurr January 29, 2024 22:10

[FOLD] Fix linux compile

4a9af0a

intelliot added Bug Amendment Perf impact not expected Change is not expected to improve nor harm performance. labels Jan 30, 2024

seelabs reviewed Jan 31, 2024

View reviewed changes

src/ripple/app/misc/impl/AMMUtils.cpp Outdated Show resolved Hide resolved

intelliot added this to the 2.1.0 (Mar 2024) milestone Feb 1, 2024

intelliot added the Testable label Feb 1, 2024

gregtatcam added 2 commits February 1, 2024 09:53

[FOLD] Construct inner objects with STObject::makeInnerObject()

51fc002

[FOLD] Fix clang format

51eabc4

gregtatcam mentioned this pull request Feb 1, 2024

PriceOracle: Price Oracle (XLS-47d) #4789

Merged

12 tasks

ximinez requested changes Feb 1, 2024

View reviewed changes

src/ripple/app/misc/impl/AMMUtils.cpp Outdated Show resolved Hide resolved

src/ripple/protocol/impl/STObject.cpp Outdated Show resolved Hide resolved

src/ripple/protocol/impl/STObject.cpp Show resolved Hide resolved

src/ripple/protocol/SOTemplate.h Show resolved Hide resolved

[FOLD] Address reviewer's feedback

292772c

* Add comment to InnerObjectFormats * Use if initializer in makeInnerObject() * Add assert to STObject::set()

ximinez approved these changes Feb 1, 2024

View reviewed changes

src/ripple/app/misc/impl/AMMUtils.cpp Outdated Show resolved Hide resolved

[FOLD] Handle AuthAccounts on empty state deposit; update unit-tests

034b98e

gregtatcam added 2 commits February 5, 2024 10:44

Merge branch 'develop' into fix-innerobj-templ

aa5c20a

[FOLD] Revert AuthAccounts change and other fixes

87a2879

* Create AuctionSlot on AMMCreate and update on AMMDeposit * Add assert to check for AuctionSlot

gregtatcam requested a review from ximinez February 6, 2024 19:14

seelabs reviewed Feb 6, 2024

View reviewed changes

gregtatcam added 2 commits February 6, 2024 19:16

[FOLD] Address @seelabs feedback

4e6f210

* Remove comments * Refactor amendment changed code

[FOLD] Address @seelabs feedback

0013fde

* Change STObject::set() arg to rvalue ref

seelabs approved these changes Feb 7, 2024

View reviewed changes

ximinez requested changes Feb 7, 2024

View reviewed changes

src/ripple/app/misc/impl/AMMUtils.cpp Outdated Show resolved Hide resolved

src/test/jtx/impl/AMM.cpp Show resolved Hide resolved

src/test/jtx/impl/AMM.cpp Outdated Show resolved Hide resolved

[FOLD] Address @ximinez feedback

98d0a19

* Refactor auction slot amendment creation on initialize * Update a comment in amm test utils

intelliot requested review from seelabs and ximinez February 7, 2024 19:06

seelabs approved these changes Feb 7, 2024

View reviewed changes

ximinez approved these changes Feb 7, 2024

View reviewed changes

Merge branch 'develop' into fix-innerobj-templ

f2677a3

intelliot merged commit be12136 into XRPLF:develop Feb 7, 2024
17 checks passed

gregtatcam mentioned this pull request Feb 7, 2024

Proposed 2.1.0-rc1 #4910

Merged

1 task

intelliot changed the title ~~Add STObject constructor to explicitly set inner object template~~ fixInnerObjTemplate: Add STObject constructor to explicitly set inner object template Feb 8, 2024

intelliot assigned legleux Feb 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`fixInnerObjTemplate`: Add STObject constructor to explicitly set inner object template #4906

`fixInnerObjTemplate`: Add STObject constructor to explicitly set inner object template #4906

gregtatcam commented Jan 29, 2024 •

edited by ximinez

Loading

codecov-commenter commented Jan 30, 2024 •

edited

Loading

ximinez left a comment

danielwwf commented Feb 3, 2024 •

edited

Loading

gregtatcam commented Feb 6, 2024

seelabs left a comment

dangell7 commented Feb 7, 2024

gregtatcam commented Feb 7, 2024

dangell7 commented Feb 7, 2024

intelliot commented Feb 7, 2024

intelliot commented Feb 17, 2024

fixInnerObjTemplate: Add STObject constructor to explicitly set inner object template #4906

fixInnerObjTemplate: Add STObject constructor to explicitly set inner object template #4906

Conversation

gregtatcam commented Jan 29, 2024 • edited by ximinez Loading

High Level Overview of Change

Context of Change

Type of Change

Test Plan

codecov-commenter commented Jan 30, 2024 • edited Loading

Codecov Report

ximinez left a comment

Choose a reason for hiding this comment

danielwwf commented Feb 3, 2024 • edited Loading

gregtatcam commented Feb 6, 2024

seelabs left a comment

Choose a reason for hiding this comment

dangell7 commented Feb 7, 2024

gregtatcam commented Feb 7, 2024

dangell7 commented Feb 7, 2024

intelliot commented Feb 7, 2024

intelliot commented Feb 17, 2024

`fixInnerObjTemplate`: Add STObject constructor to explicitly set inner object template #4906

`fixInnerObjTemplate`: Add STObject constructor to explicitly set inner object template #4906

gregtatcam commented Jan 29, 2024 •

edited by ximinez

Loading

codecov-commenter commented Jan 30, 2024 •

edited

Loading

danielwwf commented Feb 3, 2024 •

edited

Loading