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

Tweak self-transfer behaviour to match v15 #364

Closed
wants to merge 1 commit into from
Closed

Conversation

arajasek
Copy link
Contributor

@arajasek arajasek commented Mar 7, 2022

Closes #143

Matches behaviour in filecoin-project/lotus#7637.

The motivation for this change is mostly around UX -- it appears weird when people successfully send themselves 2B Filecoin, even though no actual transfer occurs. This change is slightly less efficient than exiting early on self-transfers, but that's not really a case we're looking to optimize.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM. Do we know of any such messages on-chain? I.e., should this be behind a version guard?

NOTE: we can always remove such version guards later, but it's still useful to be able to sync/debug against chain history for now.

@@ -214,6 +210,15 @@ where
.context("cannot transfer from non-existent sender")
.or_error(ErrorNumber::InsufficientFunds)?;

if from_actor.balance.lt(value) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we not just use from_actor.balance < value?

@jennijuju
Copy link
Member

LGTM. Do we know of any such messages on-chain? I.e., should this be behind a version guard?

NOTE: we can always remove such version guards later, but it's still useful to be able to sync/debug against chain history for now.

I.e https://m.filscout.com/zh/message/bafy2bzacebr2quxfina5pcy3uudh5lhrbapmbdaxd3l5qfkfhaun6yq44qxmu

@Stebalien
Copy link
Member

Heh. Lol. But if there's nothing recent, we're probably fine merging without a guard (this is just for testing).

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Let's add a version guard. ref-fvm officially supports nv14 and above, and this would break support for nv14. (And we should extract a test vector from that message.)

@arajasek
Copy link
Contributor Author

arajasek commented Mar 9, 2022

Yeah, opened this before the discussion that resulted in #360 (comment)

@arajasek
Copy link
Contributor Author

arajasek commented Mar 9, 2022

Pulling into #360

@arajasek arajasek closed this Mar 9, 2022
@Stebalien Stebalien deleted the asr/self-send branch August 5, 2022 00:28
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.

make sure the Machine's transfer logic is level with nv15
4 participants