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: return tx block height info #383

Closed
wants to merge 2 commits into from

Conversation

mFragaBA
Copy link
Contributor

@mFragaBA mFragaBA commented Jun 12, 2024

addresses #380.

Includes the block where a transaction was included and the account it was executed against in the state sync response. In this PR, we include that information within the existing AccountSummary. A few issues arise from this though:

  • first, I couldn't just modify the AccountSummary protobuf message as it's reused in other places. Hence I made a wrapper named AccountUpdate (similar to the NullifierUpdate name).
    • AccountUpdate contains an AccountSummary and a list of TransactionInfo (which in turn contains the transaction ID and block number)
  • I had to change a bit select_transactions_by_accounts_and_block_range. Now it returns a mapping account_id => TransactionInfo
  • Then, on the sync state handler we need to "merge" the TransactionInfos with the fetched account summaries. This poses a new issue as some transactions might not have a matching account summary if a client with old state does a sync request (@bobbinth mentioned situation where this happens in the issue).
    • By just wrapping the AccountSummary struct, one problem is that the account_id for AccountUpdate is within the optional AccountSummary. So that will require to either modify AccountSummary, to define a new message that stores almost the same info as AccountSummary or to embed the fields within AccountUpdate

Note: This is a POC implementation, made so we can compare this vs. a decoupled approach.

TODO:

  • code cleanup
  • update documentation

Sorry, something went wrong.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@bobbinth
Copy link
Contributor

Note: This is a POC implementation, made so we can compare this vs. a decoupled approach.

Could you create a PR for the "decoupled approach" as well? Would make it a bit easier to compare the two.

@mFragaBA
Copy link
Contributor Author

Note: This is a POC implementation, made so we can compare this vs. a decoupled approach.

Could you create a PR for the "decoupled approach" as well? Would make it a bit easier to compare the two.

#386 done!

@mFragaBA mFragaBA force-pushed the mFragaBA-return-tx-info-on-account-summary branch 5 times, most recently from e48a2e3 to 94dd35f Compare June 13, 2024 21:12
@mFragaBA mFragaBA force-pushed the mFragaBA-return-tx-info-on-account-summary branch from 94dd35f to 2cac984 Compare June 13, 2024 22:13
@mFragaBA
Copy link
Contributor Author

closing this PR since we ended up going with the decoupled approach on #386

@mFragaBA mFragaBA closed this Jun 18, 2024
@bobbinth bobbinth deleted the mFragaBA-return-tx-info-on-account-summary branch July 4, 2024 19:25
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.

None yet

2 participants