-
Notifications
You must be signed in to change notification settings - Fork 985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Contract data #3439
Contract data #3439
Conversation
7a974e5
to
d26901d
Compare
78fcbd3
to
098369c
Compare
270551f
to
d2be27e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is almost mergeable. I'd really like if we could clean up the history a bit, because it will make things easier to understand when we thoroughly review everything before we release.
892ae2f
to
e1160f2
Compare
9022dcc
to
b2e8e17
Compare
cb6fc79
to
807a3cf
Compare
return true; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which code path ends up calling this for Jump Cannon?
dde6a17
to
81ce755
Compare
I believe I've now addressed all review comments and have updated the graydon/rs-stellar-xdr/contract-data and stellar/rs-stellar-contract-env/contract-data branches that depend on this (and that this depends on cyclically). @jonjove @jayz22 @leighmcculloch @MonsieurNicolas could you please mark comments as resolved that have been resolved? (and if there's nothing else really critical, can someone r+?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved most comments, I only have one left
return true; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's actually more conservative to do what we do for LIQUIDITY_POOL
here (and throw): we should not have any dependency on "SponsorshipUtils" from Jump Cannon.
I don't have the permission to resolve comments, but feel free to resolve them yourself. They are minor and you've clarified them both. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three very minor comments then let's get this merged.
src/test/TestUtils.cpp
Outdated
@@ -114,6 +115,10 @@ computeMultiplier(LedgerEntry const& le) | |||
case OFFER: | |||
case DATA: | |||
return 1; | |||
#ifdef ENABLE_NEXT_PROTOCOL_VERSION_UNSAFE_FOR_PRODUCTION | |||
case CONTRACT_DATA: | |||
return 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that @MonsieurNicolas wants to have no dependency between jump cannon and sponsorships, should we throw on this case as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@graydon think you missed this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, fixed.
Pushed update containing all review comments from @jonjove and @MonsieurNicolas today. |
r+ f013c99 |
This is a consolidated branch including changes for CAP-0047 and 0053 (covering CONFIG_SETTING and CONTRACT_DATA ledger entries, and all support code).