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

Attestation improvements #2559

Merged
merged 6 commits into from
Dec 19, 2024
Merged

Attestation improvements #2559

merged 6 commits into from
Dec 19, 2024

Conversation

onnovisser
Copy link
Collaborator

@onnovisser onnovisser commented Dec 17, 2024

Description

#2557

  • Include onchainReserve in the assets-section - TAKE from runtimeApi Nav.reserve
  • Include ALL assets in the assets-section - currently, cash items are missing
  • Include total accrued-unpaid fees as 1 asset item - price = -1 with right decimals, TAKE from runtimeApi Nav.navFees
  • Adjust quantity field to pool decimals - currently, it is 18 decimals
  • Change asset structure
    • assetassetId - 0 for onchainReserve
    • New assetname retrieved for the asset from ipfs data, onchainReserve for onchainReserve
  • Remove netFeeValue
  • Add "tokenAddresses" → list addresses of tranche tokens on the relevant networks, identified by chainId - see json for details
  • Add timestamp when this was signed to portfolio - unix timestamp in seconds
  • Adapt signature to be a struct containing signature and publicKey of signer
  • Ensure netAssetValue is correct - TAKE from runtimeApi Nav.total

Approvals

  • Dev

Screenshots

Impact

Copy link

github-actions bot commented Dec 17, 2024

PR deployed in Google Cloud
URL: https://app-pr2559.k-f.dev
Commit #: f5ca40c
To access the functions directly check the corresponding deploy Action

Copy link

github-actions bot commented Dec 17, 2024

PR deployed in Google Cloud
URL: https://pr2559-app-ff-production.k-f.dev
Commit #: f5ca40c
To access the functions directly check the corresponding deploy Action


let signature: { hash: string; publicKey: string } | null = null
try {
const message = JSON.stringify(attestation)
Copy link
Contributor

@hieronx hieronx Dec 17, 2024

Choose a reason for hiding this comment

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

It is now signing { portfolio: { ... } }. It would be cleaner if it's just signing the inner part, so just { ... }, and then it's combined into a new object that is

{
  "portfolio": { ... },
  "signature": { ... }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I can make this change in the next few days though, don't worry about it :)

Copy link
Contributor

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

Not blocking with change request, but needs one change. Otherwise, thanks for the awesome work so close before the holidays!

Comment on lines +178 to +181
quantity: CurrencyBalance.fromFloat(
l.pricing.outstandingQuantity.toDecimal(),
pool.currency.decimals
).toString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to check how that rounds? ^^

Copy link
Contributor

@hieronx hieronx left a comment

Choose a reason for hiding this comment

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

LGTM!

@hieronx hieronx enabled auto-merge (squash) December 19, 2024 12:52
@hieronx hieronx merged commit 254503b into main Dec 19, 2024
11 checks passed
@hieronx hieronx deleted the attestation-tweaks branch December 19, 2024 12: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.

3 participants