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

ERTP: payment ledger can just store values #3167

Open
katelynsills opened this issue May 24, 2021 · 8 comments · May be fixed by #6432
Open

ERTP: payment ledger can just store values #3167

katelynsills opened this issue May 24, 2021 · 8 comments · May be fixed by #6432
Assignees
Labels
ERTP package: ERTP patterns vaults_triage DO NOT USE

Comments

@katelynsills
Copy link
Contributor

What is the Problem Being Solved?

The paymentLedger in ERTP is the mapping of payments to the current amount of digital assets they hold. All of the issuer methods regarding payments use amounts (brand + value), but that does not imply that the storage itself needs to be in terms of amounts, since the brand is the same for all. Storing just the values would potentially save space and only add a few more lines of code to get the value from an amount on entry of a method and reconstitute an amount on return for each method.

Description of the Design

Store values in paymentLedger, use helpers to switch from amounts to values and vice versa.

Security Considerations

We should make sure that the brand in amounts coming from users (uncoerced amounts) is always checked and never assumed.

Test Plan

The tests that already exist should cover this well, but targeted unit tests would be helpful too.

Scheduling Considerations

It would be nice to schedule before audits.

@katelynsills katelynsills added ERTP package: ERTP Zoe package: Zoe labels May 24, 2021
@katelynsills katelynsills self-assigned this May 24, 2021
@erights
Copy link
Member

erights commented May 25, 2021

As stated, this is a good plan in general, because all the amounts for a given issuer all have the same brand. I just want to note, to avoid any misunderstanding, that this does not avoid storing brands. The values may contain brands. In particular, invitations often do.

@katelynsills
Copy link
Contributor Author

As stated, this is a good plan in general, because all the amounts for a given issuer all have the same brand. I just want to note, to avoid any misunderstanding, that this does not avoid storing brands. The values may contain brands. In particular, invitations often do.

Yup, the values may include many presences, including brands.

@Tartuffo
Copy link
Contributor

@kriskowal @mhofman Does this still need to get done? For MN-1?

@erights
Copy link
Member

erights commented Jan 21, 2022

It's a nice optimization. Could even be important since we'll have a zillion payments. Please leave open but not urgent.

@Tartuffo
Copy link
Contributor

So just to be clear: "not urgent" means Not Mainnet 1, right?

@erights
Copy link
Member

erights commented Jan 22, 2022

yes

@kriskowal kriskowal assigned erights and unassigned kriskowal and mhofman Jan 22, 2022
@kriskowal
Copy link
Member

I’ve reassigned to @erights in hope that we can find someone who’s brain is closer to this problem than myself or @mhofman.

@Tartuffo Tartuffo removed the Small label Apr 8, 2022
@erights erights linked a pull request Oct 13, 2022 that will close this issue
@erights
Copy link
Member

erights commented Oct 13, 2022

Would be fixed by #6432

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ERTP package: ERTP patterns vaults_triage DO NOT USE
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants