Skip to content

off-by-one #1747

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.

Sign up for GitHub

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

Jump to bottom
Merged
ukby1234 merged 5 commits into from
Jan 5, 2022
Merged

off-by-one #1747

ukby1234 merged 5 commits into from
Jan 5, 2022

Conversation

@ukby1234
Copy link
Contributor

@ukby1234 ukby1234 commented Jan 4, 2022

always transfer one more ksm to fix rounding issue

@shengda

@ukby1234 ukby1234 requested a review from xlc January 4, 2022 08:49
xlc
xlc reviewed Jan 4, 2022
View reviewed changes
Copy link
Member

@xlc xlc left a comment

Choose a reason for hiding this comment

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

Better to have some integration test to cover this kind of errors.
Also should use saturated add

@ukby1234
Copy link
Contributor Author

ukby1234 commented Jan 4, 2022

added integration tests. I don't think saturated add is the right semantics here because we do want to ensure the converted back balance >= the original balance. This means if +1 is an overflow, it should fail.

@ukby1234 ukby1234 enabled auto-merge (squash) January 4, 2022 22:31
@xlc
Copy link
Member

xlc commented Jan 4, 2022

But you are using unwrap_or_default so overflow become 0, which is more wrong than max value.

@ukby1234
Copy link
Contributor Author

ukby1234 commented Jan 4, 2022

Yeah, but when it becomes 0, our program definitely won't work on the current transaction instead of failing on possible later transactions. This gives us much more visibility.

@xlc
Copy link
Member

xlc commented Jan 4, 2022

ok. in that case can you add some comments in the code to explain why return 0 on edge case is better? also should use and_then instead of multiple unwrap_of_default

xlc
xlc reviewed Jan 4, 2022
View reviewed changes
Frank Yin and others added 2 commits January 4, 2022 15:26
@ukby1234 ukby1234 merged commit df09171 into master Jan 5, 2022
@ukby1234 ukby1234 deleted the off-by-one branch January 5, 2022 00:32
syan095 pushed a commit that referenced this pull request Jan 7, 2022
Merge remote-tracking branch 'origin/master' into fix/test-coverage
ae57f42 
* origin/master: (102 commits)
  Fix collect_fee (#1754)
  Update HEADER-GPL3
  Update extrinsic-ordering-check-from-bin.yml (#1752)
  Update HEADER-GPL3
  bump version (#1751)
  Remove homa-lite from karura runtime (#1744)
  off-by-one (#1747)
  Revert "simulate exchange rate (#1742)" (#1746)
  simulate exchange rate (#1742)
  bump version (#1743)
  refactor homa (#1648)
  Update stable asset (#1741)
  add more info to events (#1740)
  Fix mandala swap path error (#1736)
  update stable asset (#1738)
  remove unnecessary code (#1735)
  fix currency id testing (#1733)
  rework fee payment (#1687)
  Add Deposit for Setting Alternative Fee Swap Path (#1730)
  Add register_erc20_asset and update_erc20_asset (#1731)
  ...

# Conflicts:
#	.github/workflows/coverage.yml.disabled
#	Cargo.lock
#	Cargo.toml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xlc xlc xlc approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ukby1234 @xlc