Skip to content

fix: block parsing logic for public credentials#697

Merged
ntn-x2 merged 8 commits into
developfrom
aa/public-credentials-block-parsing-fix
Dec 16, 2022
Merged

fix: block parsing logic for public credentials#697
ntn-x2 merged 8 commits into
developfrom
aa/public-credentials-block-parsing-fix

Conversation

@ntn-x2
Copy link
Copy Markdown
Contributor

@ntn-x2 ntn-x2 commented Dec 15, 2022

Previous logic was assuming that the only extrinsic that could create a public credential was a submit_did_call extrinsic. This is not true, as batch, batchAll, and forceBatch can also create a public credential.

This is now taken into account in this PR, and an integration test that covers the new case has been added.

One problematic block, raised by BTE, was https://kilt-testnet.subscan.io/block/2128207.

@ntn-x2 ntn-x2 self-assigned this Dec 15, 2022
@rflechtner
Copy link
Copy Markdown
Contributor

I'd have to take more time to give qualified feedback, but looks reasonable on first glance.
I have to say though it still seems wild to me that we do this instead of, say, firing events with the credential in them. I did expect issues like this to come up when I first heard of it, and here we are 🥹
Are we sure this is the best way of doing this?

@ntn-x2
Copy link
Copy Markdown
Contributor Author

ntn-x2 commented Dec 16, 2022

@rflechtner I understand and I agree it would make things easier, but remember that the credential is user-provided input. Yes, it is bound in the maximum size it can have, but the general feeling also on the forums is that events should be kept small and only used as index to something else in the state (e.g., https://substrate.stackexchange.com/questions/4296/large-event-payloads-unsafe).
In our case, we don't store the credential input in the state, hence this approach does not apply to us. Furthermore, events are written on storage and cleaned on the next block, so that means that larger event payload would have an even bigger impact on the weight of the extrinsic. I know this is probably not the cleanest way to access the information, but I think it's the best tradeoff. Perhaps, once back, @weichweich might want to chip in as well, since we discussed also in the past how we can use the block number as a pointer and it never came up to put the whole payload in an event body.

@ntn-x2 ntn-x2 mentioned this pull request Dec 16, 2022
3 tasks
@ntn-x2 ntn-x2 force-pushed the aa/public-credentials-block-parsing-fix branch 2 times, most recently from 10c4a93 to 63e2c79 Compare December 16, 2022 11:57
@ntn-x2 ntn-x2 force-pushed the aa/public-credentials-block-parsing-fix branch from 63e2c79 to b756f70 Compare December 16, 2022 12:00
Comment thread packages/core/src/publicCredential/PublicCredential.chain.ts Outdated
Comment thread packages/core/src/publicCredential/PublicCredential.chain.ts
Comment thread packages/core/src/publicCredential/PublicCredential.chain.ts Outdated
@ntn-x2
Copy link
Copy Markdown
Contributor Author

ntn-x2 commented Dec 16, 2022

@rflechtner since this is blocking BTE, I've decided to go on and merge it after getting a review from Tom. Anything else you wanted to discuss about this, we can do once we're both back, and address any further open points.

@ntn-x2 ntn-x2 merged commit 92c90f6 into develop Dec 16, 2022
@ntn-x2 ntn-x2 deleted the aa/public-credentials-block-parsing-fix branch December 16, 2022 15:24
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