Skip to content

chore: update boxo to version with fewer globals#9907

Merged
Jorropo merged 3 commits intomasterfrom
feat/boxo-update-fewer-globals
Jun 8, 2023
Merged

chore: update boxo to version with fewer globals#9907
Jorropo merged 3 commits intomasterfrom
feat/boxo-update-fewer-globals

Conversation

@aschmahmann
Copy link
Contributor

@aschmahmann aschmahmann commented May 30, 2023

Validating ipfs/boxo#324 which collects the changes to resolve ipfs/boxo#291

@aschmahmann aschmahmann requested a review from a team as a code owner May 30, 2023 20:39
@aschmahmann aschmahmann force-pushed the feat/boxo-update-fewer-globals branch 2 times, most recently from 73e1ef4 to cfa056d Compare May 30, 2023 20:54
return err
}

blockDecoder := ipldlegacy.NewDecoder()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The use a global here (i.e. the go-ipld-prime defaults) is pretty unfortunate and subtle. However, it should be ok.

Note: previously this decoder included the registration of the go-ipld-format Raw and Dag-Pb converters. However, IIUC that shouldn't be necessary in this function since the go-codec-dagpb codec and the built-in go-ipld-prime raw codec should be sufficient.

@aschmahmann aschmahmann force-pushed the feat/boxo-update-fewer-globals branch from dbfb4e9 to 8e917b6 Compare June 2, 2023 13:27
@aschmahmann aschmahmann force-pushed the feat/boxo-update-fewer-globals branch 2 times, most recently from 670d307 to 624267d Compare June 8, 2023 05:07
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

(Meta boxo comment) It is sad to see a consumer go through breaking changes update for what is a code we want to kill.

@aschmahmann aschmahmann force-pushed the feat/boxo-update-fewer-globals branch from 624267d to 4cde204 Compare June 8, 2023 06:10
Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

sgtm

@aschmahmann
Copy link
Contributor Author

A couple notes:

  1. rename dagpb protobuf to merkledag.v1 boxo#323 needs a review + merge first. Although this PR seems to validate that that one is fine.
  2. After the above PR is merged this one can be too, or it can be updated once the boxo release is cut depending on when that's happening.

@Jorropo Jorropo force-pushed the feat/boxo-update-fewer-globals branch from 4cde204 to 5fbe8c6 Compare June 8, 2023 07:30
@Jorropo Jorropo enabled auto-merge (rebase) June 8, 2023 07:30
@Jorropo Jorropo merged commit de59ac1 into master Jun 8, 2023
@Jorropo Jorropo deleted the feat/boxo-update-fewer-globals branch June 8, 2023 07:46
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.

go-merkledag duplicates and global variables

3 participants