Skip to content
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

Respect trustline limit in to_classic conversions #516

Merged
merged 3 commits into from
Oct 5, 2022

Conversation

dmkozh
Copy link
Contributor

@dmkozh dmkozh commented Oct 3, 2022

What

Respect trustline limit in to_classic conversions

Why

This is to respect the trustline invariant of balance not exceeding the limit.

Known limitations

N/A

Copy link
Contributor

@sisuresh sisuresh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just wanted to point out that to_classic was renamed to export.

@dmkozh
Copy link
Contributor Author

dmkozh commented Oct 3, 2022

LGTM. Just wanted to point out that to_classic was renamed to export.

Sure, I'm aware of this. That's just a cherry-pick from an older branch.

Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question that comes from my lack of knowledge about how this works in stellar-core, so no need to block merging.

@@ -1002,6 +1002,7 @@ impl Host {
let max_balance = i64::MAX;
(min_balance, max_balance)
};
let max_balance = min(max_balance, tl.limit);
Copy link
Member

@leighmcculloch leighmcculloch Oct 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This excludes the buying liabilities from the limit, rather than reduces the limit by the buying liabilities. Is that correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems suspicious to me because this would, I think, mean that you could exceed your limit by canceling things that create buying liabilities.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you're right. max_balance should be initialized with tl.limit instead of i64::MAX.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right, changed this to apply limit before liabilities.

Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Is this being tested in the follow testing PR referenced previously? @sisuresh can this be merged now?

@dmkozh
Copy link
Contributor Author

dmkozh commented Oct 4, 2022

This looks good to me. Is this being tested in the follow testing PR referenced previously? @sisuresh can this be merged now?

Sure, I've found all these issues while writing the respective tests. That said, #515 needs to be merged before the tests.

@sisuresh sisuresh enabled auto-merge (squash) October 4, 2022 23:57
@sisuresh sisuresh merged commit ae8dfb8 into stellar:main Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants