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

feat: initial 0.5.0 support #87

Merged
merged 11 commits into from
Mar 19, 2024
Merged

feat: initial 0.5.0 support #87

merged 11 commits into from
Mar 19, 2024

Conversation

TimoGlastra
Copy link
Member

Needs Sphereon-Opensource/OID4VC#87 to properly work

Also probably want to wait for stable 0.5.0 but everything is working (with a few minor hacks)

Signed-off-by: Timo Glastra <[email protected]>
Comment on lines 2 to 3
// TODO: why is this needed since update to 0.5.0?
import 'fast-text-encoding'
Copy link
Member Author

Choose a reason for hiding this comment

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

Still need to figure out what library is using TextEncoder / TextDecoder.

Askar used it in 0.1.0 and that's why it was imported, but not sure why it's needed now

@TimoGlastra
Copy link
Member Author

@janrxyz I've updated everything for Credo 0.5 and also updated Expo to 50. However there seems to be something wrong with the styling now in Tamagui. And TBH I understand very little of how it all works.

Could you maybe take a look and see why it's behaving weirdly? Last thing to do before we can release a new version

Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
@janrtvld
Copy link
Contributor

janrtvld commented Mar 18, 2024

@TimoGlastra What seems to be the issue? It looks okay and the ci also succeeds

@TimoGlastra
Copy link
Member Author

Did you run it? The styling is definitely off if you compare it with the version from the app store

TimoGlastra and others added 2 commits March 18, 2024 13:01
Copy link
Contributor

@janrtvld janrtvld left a comment

Choose a reason for hiding this comment

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

There are lots of TODOs and FIXMEs in t here, hard to judge for me which are need now or later. Maybe add clarifying comments or add them to a sprint board for later.

.github/workflows/continuous-integration.yaml Show resolved Hide resolved
.github/workflows/continuous-integration.yaml Outdated Show resolved Hide resolved

// TODO: rotate the old wallet key to a new raw key
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you checked the TODOs? Is this for now or later?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a new todo for later. I discovered we are using uuids as the wallet key, while it's more secure and faster to generate a RAW wallet key if we're storing them in the secure enclave anyway.

However migrating old wallets to this new method is more work, so I stopped on it as I was getting distracted with the main purpose of this PR.
Should be a ticket though

packages/agent/package.json Show resolved Hide resolved
}
}

// NOTE: logo is used in issuer display (not sure if that's right though)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would that not be right? Should it be in the credential display?

Copy link
Member Author

Choose a reason for hiding this comment

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

Both issuer and credential have a logo. We use the logo from the issuer, and the background_image from the credential.

But in that case we don't render the logo of the credential (then we could have two logos). So more a note that we don't render it at the moment

Copy link
Member Author

Choose a reason for hiding this comment

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

Both issuer and credential have a logo. We use the logo from the issuer, and the background_image from the credential.

But in that case we don't render the logo of the credential (then we could have two logos). So more a note that we don't render it at the moment

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Would have to think about where we place this. Is this credential logo new? Have you seen other implementations on how people render it? It's not on our original design or sphereons (maybe they updated it).

// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { _sd_alg, _sd_hash, iss, vct, cnf, ...visibleProperties } = decodedPayload

// TODO: display somehow which fields can be selective disclosed
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we track this in code, or should we add it to a sprint board somewhere? I was testing with the wallet the other day and also saw that predicates for didcomm presentation requests are also not correct. Might be good to fix this sometime.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah that predicates are not correct should not happen. Can you describe what the exact issue is?

I think we should start tracking these as tickets as well yes. I'll do a pass soon to get all these tickets in

Copy link
Contributor

Choose a reason for hiding this comment

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

It's this FIXME:

// FIXME: we need to still filter based on the predicate (e.g. age is actually >= 18)

packages/agent/src/index.ts Show resolved Hide resolved
packages/agent/src/parsers.ts Outdated Show resolved Hide resolved
@TimoGlastra
Copy link
Member Author

There are lots of TODOs and FIXMEs in t here, hard to judge for me which are need now or later. Maybe add clarifying comments or add them to a sprint board for later.

Yeah fair. All of them are not for now, I'll add them to the board

@TimoGlastra TimoGlastra requested a review from janrtvld March 19, 2024 09:44
Signed-off-by: Timo Glastra <[email protected]>
@TimoGlastra TimoGlastra merged commit e1097f9 into main Mar 19, 2024
1 check passed
@TimoGlastra TimoGlastra deleted the feat/050 branch March 19, 2024 11:52
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.

2 participants