-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Dchain module: initial release & Appnexus bid adapter: partial dchain support #7473
Conversation
if (deepAccess(bid, 'meta.networkId') && deepAccess(bid, 'meta.networkName')) { | ||
basicDchain.nodes.push({ name: bid.meta.networkName, bsid: bid.meta.networkId.toString() }); | ||
} | ||
basicDchain.nodes.push({ name: bid.bidderCode }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this is some support for taking existing meta field and making it a dchain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if the adapter set dchain it is ignored?
If so, should we check the response for dchain and if not set the basic dchain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess it probably is not that big of a deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory - if a bidder supports the dchain properly the above information and any other links that were part of the buying process would already be provided in the dchain object that they attached to their bid object (through their adapter code). In that scenario, the basic dchain made here isn't needed.
The basic dchain would only be used if the bidder didn't assign dchain themselves. We try to include information that would hopefully (but not necessarily) be there.
modules/dchain.js
Outdated
} | ||
} else { | ||
// bid.meta.dchain = basicDchain; // should we assign a backup dchain if bidder's dchain was invalid? | ||
delete bid.meta.dchain; // or delete the bad object? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in my opinion we should not since if the adapter set some bad dchain we want them to fix it. So a pub should see the console error / warns and also notice dchain is never set by the adapter which sets the bad dchain.
I like how it is. (Just remove the comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove the comments here and leave the delete statement. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, just a couple comments
@jsnellbaker it seems the brandid merge created a conflict |
Thanks @patmmccann for the heads-up. I have resolved the conflict. |
If it was not defined, the dchain will create a default dchain object for prebid. | ||
|
||
## Validation modes | ||
- ```strict```: It is the default validation mode. In this mode, dchain object will not be accpeted from adapters if it is invalid. Errors are thrown for invalid dchain object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accpeted typo
Type of change
Description of change
Related to #7402 (see this link for details for functionality - #7402 (comment))
This PR adds the dchain module and adds partial support for the dchain object in the appnexus bid adapter.
To note - I'm not entirely 100% about some of the fields that were defined in the dchain module (such as for the back-up dchain object here or for the appnexus dchain object. If they should be different, please let me know and I'll correct them.
IAB Dchain Spec here:
https://iabtechlab.com/wp-content/uploads/2021/06/DemandChainObject-1.0-June2021.pdf