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

DEEP_DIVE.md demo no longer behaves as described #1432

Open
brson opened this issue Jan 16, 2024 · 4 comments
Open

DEEP_DIVE.md demo no longer behaves as described #1432

brson opened this issue Jan 16, 2024 · 4 comments

Comments

@brson
Copy link
Contributor

brson commented Jan 16, 2024

The descriptions of what the user should see in DEEP_DIVE.md no longer correspond to what actually happens. I take it this is because the data model has changed since the original demo and updates to the demo have not preserved similar behavior.

Below are the steps that didn't work as expected and potential resolutions based on my skimming of the original demo walkthrough video


"You will see the second transfer is rejected with an error for tripping the debit reserved limit."

No transfer is rejected. The concept of "reserved limit" doesn't appear to exist.

The demo does place a "debits_must_not_exceed_credits" flag on account 1, but these transfers do not trip that flag (pending + posted debits is exactly equal to credits). To make this an error again perhaps the pending debits can be increased.


"You will see these two-phase transfers only update the reserved inflight limits."

I didn't understand the phrase "reserved inflight limits", probably because the concept has changed. Maybe "reserved inflight limits" should be change to "pending credits and debits".


Again, the second transfer is rejected because it was never created.

zig/zig run src/demos/demo_05_post_pending_transfers.zig --deps vsr --mod vsr::src/vsr.zig
# At this point, Account[1] has reached its credit limit (no more debit transfers allowed).
zig/zig run src/demos/demo_02_lookup_accounts.zig --deps vsr --mod vsr::src/vsr.zig

The statement about the second transfer rejected (same as previous step) is not true.

The statement in the comment about reaching its credit limit is not true (I think). It has a debits_posted of 9500 and credits_posted of 10000, so I guess it still has 500 credit.

The reason for this seems to be that demo_05_post_pending_transfers is not commiting all of the pending transfers. Transfers 1001, 1002, and 1003 are pending, but it only commits 1001 and 1002.

This could again be fixed by tweaking the numbers, taking into account that the previous step is supposed to fail but doesn't.


Let's also pretend someone else tried to void the pending transfer concurrently:

zig/zig run src/demos/demo_06_void_pending_transfers.zig --deps vsr --mod vsr::src/vsr.zig

This succeeds when it seems to imply that it should not (in the video it fails). This is because the previous step only partially commited the pending transfers. If the previous step is fixed this will fail as expected.

@sentientwaffle
Copy link
Member

@brson Was this all resolved by #1431?

@jorangreef
Copy link
Member

No transfer is rejected. The concept of "reserved limit" doesn't appear to exist.

Good catch!

I didn't understand the phrase "reserved inflight limits", probably because the concept has changed. Maybe "reserved inflight limits" should be change to "pending credits and debits".

Yes, the "reserved" concept is now "pending". In other words, for a two-phase transfer, the debits/credits must first be reserved (in the first phase) as pending, and then (in the second phase) they can be posted/voided.

Would be great to bring this up to date.

@jorangreef
Copy link
Member

(As backstory, we moved away from words like reserved/commit in relation to two-phase transfers, because these were overloaded, e.g. with reserved fields, or with the commit message in a consensus protocol)

@brson
Copy link
Contributor Author

brson commented Jan 23, 2024

@brson Was this all resolved by #1431?

No, that PR just fixes technical build steps.

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

No branches or pull requests

3 participants